Date:   Wed, 4 Nov 2020 17:53:46 -0800
From:   harshad shirwadkar <harshadshirwadkar@...il.com>
To:     Jan Kara <jack@...e.cz>
Cc:     Ext4 Developers List <linux-ext4@...r.kernel.org>,
        "Theodore Y. Ts'o" <tytso@....edu>
Subject: Re: [PATCH 03/10] ext4: pass handle to ext4_fc_track_* functions

On Tue, Nov 3, 2020 at 6:46 AM Jan Kara <jack@...e.cz> wrote:
>
> On Sat 31-10-20 13:05:11, Harshad Shirwadkar wrote:
> > Use transaction id found in handle->h_transaction->h_tid for tracking
> > fast commit updates. This patch also restructures ext4_unlink to make
> > handle available inside ext4_unlink for fast commit tracking.
> >
> > Suggested-by: Jan Kara <jack@...e.cz>
> > Signed-off-by: Harshad Shirwadkar <harshadshirwadkar@...il.com>
>
> Thanks for the patch. Couple of comments below:
>
> > @@ -4651,8 +4652,6 @@ long ext4_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
> >                    FALLOC_FL_COLLAPSE_RANGE | FALLOC_FL_ZERO_RANGE |
> >                    FALLOC_FL_INSERT_RANGE))
> >               return -EOPNOTSUPP;
> > -     ext4_fc_track_range(inode, offset >> blkbits,
> > -                     (offset + len - 1) >> blkbits);
>
> Why do you delete the ext4_fc_track_range() call here?
I realized that all the different ext4_fallocate functions such as
punch_hole, zero_range etc have their own track calls. Collapse_range
and insert_range are fc ineligible operations, and the default
fallocate calls ext4_map_blocks(), so this call here isn't really
needed. I also took a look at all the other calls to
ext4_fc_track_range() and I think we only need ext4_fc_track_range()
calls in following situations:

1) ext4_map_blocks()
2) ext4_punch_hole()
3) ext4_zero_range()
4) ext4_setattr() --> for handling truncate

I'll remove all the other callers in the next version.

>
> > diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c
> > index 354f81ff819d..5c3af472287a 100644
> > --- a/fs/ext4/fast_commit.c
> > +++ b/fs/ext4/fast_commit.c
> > @@ -323,15 +323,18 @@ static inline int ext4_fc_is_ineligible(struct super_block *sb)
> >   * If enqueue is set, this function enqueues the inode in fast commit list.
> >   */
> >  static int ext4_fc_track_template(
> > -     struct inode *inode, int (*__fc_track_fn)(struct inode *, void *, bool),
> > +     handle_t *handle, struct inode *inode,
> > +     int (*__fc_track_fn)(struct inode *, void *, bool),
> >       void *args, int enqueue)
> >  {
> > -     tid_t running_txn_tid;
> >       bool update = false;
> >       struct ext4_inode_info *ei = EXT4_I(inode);
> >       struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
> > +     tid_t tid = 0;
> >       int ret;
> >
> > +     if (ext4_handle_valid(handle) && handle->h_transaction)
> > +             tid = handle->h_transaction->t_tid;
>
> The handle->h_transaction check is pointless here. It is always true. And
> if you move the tid fetching after the JOURNAL_FAST_COMMIT check below, you
> don't need the ext4_handle_valid() check either as fastcommit cannot be
> enabled without a journal.
Ack
>
> >       if (!test_opt2(inode->i_sb, JOURNAL_FAST_COMMIT) ||
> >           (sbi->s_mount_state & EXT4_FC_REPLAY))
> >               return -EOPNOTSUPP;
>
>
> > diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> > index 5159830dacb8..f3f8bf61072e 100644
> > --- a/fs/ext4/namei.c
> > +++ b/fs/ext4/namei.c
> > @@ -2631,7 +2631,7 @@ static int ext4_create(struct inode *dir, struct dentry *dentry, umode_t mode,
> >               inode_save = inode;
> >               ihold(inode_save);
> >               err = ext4_add_nondir(handle, dentry, &inode);
> > -             ext4_fc_track_create(inode_save, dentry);
> > +             ext4_fc_track_create(handle, inode_save, dentry);
> >               iput(inode_save);
> >       }
> >       if (handle)
> > @@ -2668,7 +2668,7 @@ static int ext4_mknod(struct inode *dir, struct dentry *dentry,
> >               ihold(inode_save);
> >               err = ext4_add_nondir(handle, dentry, &inode);
> >               if (!err)
> > -                     ext4_fc_track_create(inode_save, dentry);
> > +                     ext4_fc_track_create(handle, inode_save, dentry);
> >               iput(inode_save);
> >       }
>
> Not directly related to this patch but why do you bother with 'inode_save'
> in the above cases? I guess you're afraid by the comment that "inode
> reference is consumed by the dentry" but since you have a dentry reference
> as well, you can be sure that the inode stays around...
Yeah, I think I was confused by that. I'll get rid of inode_save stuff
and just use d_inode(dentry) instead.
>
> >       if (handle)
> > @@ -2833,7 +2833,7 @@ static int ext4_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode)
> >               iput(inode);
> >               goto out_retry;
> >       }
> > -     ext4_fc_track_create(inode, dentry);
> > +     ext4_fc_track_create(handle, inode, dentry);
> >       ext4_inc_count(dir);
>
> And I was also wondering why all the directory tracking functions take both
> dentry and the inode. You can fetch inode from a dentry with d_inode()
> helper so I don't see a reason for passing it separately. That is, in a
> couple of places you call ext4_fc_track_*() before d_instantiate[_new]() so
> dentry isn't fully setup yet but there's nothing which prevents you from
> calling it after d_instantiate().
>
> The only possible exception to this is the ext4_rename() code. There you
> don't have suitable dentry for the link tracking so this would need to
> explicitely pass the inode & dentry. But that place can just call a low
> level wrapper allowing that. All the other places can use a higher level
> function which just takes the dentry.
Yeah, it's the rename that's the problem. Rename calls
ext4_fc_track_link() and ext4_fc_track_unlink(). I'd need to define
two lower level wrappers just for this one function call. But I agree
that it will make the code look much cleaner. I'll do that in V2.
>
> >  static int ext4_unlink(struct inode *dir, struct dentry *dentry)
> >  {
> > +     handle_t *handle;
> >       int retval;
> >
> >       if (unlikely(ext4_forced_shutdown(EXT4_SB(dir->i_sb))))
> > @@ -3282,9 +3273,16 @@ static int ext4_unlink(struct inode *dir, struct dentry *dentry)
> >       if (retval)
> >               goto out_trace;
> >
> > -     retval = __ext4_unlink(dir, &dentry->d_name, d_inode(dentry));
> > +     handle = ext4_journal_start(dir, EXT4_HT_DIR,
> > +                                 EXT4_DATA_TRANS_BLOCKS(dir->i_sb));
> > +     if (IS_ERR(handle)) {
> > +             retval = PTR_ERR(handle);
> > +             goto out_trace;
> > +     }
> > +
> > +     retval = __ext4_unlink(handle, dir, &dentry->d_name, d_inode(dentry));
> >       if (!retval)
> > -             ext4_fc_track_unlink(d_inode(dentry), dentry);
> > +             ext4_fc_track_unlink(handle, d_inode(dentry), dentry);
> >  #ifdef CONFIG_UNICODE
> >       /* VFS negative dentries are incompatible with Encoding and
> >        * Case-insensitiveness. Eventually we'll want avoid
> > @@ -3295,6 +3293,8 @@ static int ext4_unlink(struct inode *dir, struct dentry *dentry)
> >       if (IS_CASEFOLDED(dir))
> >               d_invalidate(dentry);
> >  #endif
> > +     if (handle)
> > +             ext4_journal_stop(handle);
>
> How could 'handle' be NULL here?
Ack

Thanks,
Harshad

>
>                                                                 Honza
> --
> Jan Kara <jack@...e.com>
> SUSE Labs, CR

Powered by blists - more mailing lists