On Tue, Nov 3, 2020 at 6:46 AM Jan Kara <jack@xxxxxxx> 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@xxxxxxx>
> > Signed-off-by: Harshad Shirwadkar <harshadshirwadkar@xxxxxxxxx>
>
> 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@xxxxxxxx>
> SUSE Labs, CR
