On Thu, Dec 12, 2024 at 2:00 PM Jan Kara <jack@xxxxxxx> wrote:
>
> On Sun 18-08-24 04:03:49, Harshad Shirwadkar wrote:
> > If the inode that's being requested to track using ext4_fc_track_inode
> > is being committed, then wait until the inode finishes the
> > commit. Also, add calls to ext4_fc_track_inode at the right places.
> >
> > With this patch, now calling ext4_reserve_inode_write() results in
> > inode being tracked for next fast commit. A subtle lock ordering
> > requirement with i_data_sem (which is documented in the code) requires
> > that ext4_fc_track_inode() be called before grabbing i_data_sem. So,
> > this patch also adds explicit ext4_fc_track_inode() calls in places
> > where i_data_sem grabbed.
> >
> > Signed-off-by: Harshad Shirwadkar <harshadshirwadkar@xxxxxxxxx>
>
> Sorry for the huge delay! Some comments are below:
Hi Jan,
Sorry for a huge delay from my end as well. Thank you for your
comments on V7 of this patchset. I just sent out a V8 of this patchset
where I have handled all of your comments on this and subsequent
patches in the series.
Thank you,
Harshad
>
> > @@ -598,6 +601,36 @@ void ext4_fc_track_inode(handle_t *handle, struct inode *inode)
> > if (ext4_test_mount_flag(inode->i_sb, EXT4_MF_FC_INELIGIBLE))
> > return;
> >
> > + if (!list_empty(&ei->i_fc_list))
> > + return;
> > +
> > +#ifdef CONFIG_LOCKDEP
> > + /*
> > + * If we come here, we may sleep while waiting for the inode to
> > + * commit. We shouldn't be holding i_data_sem when we go to sleep since
> > + * the commit path needs to grab the lock while committing the inode.
> > + */
> > + WARN_ON(lockdep_is_held(&ei->i_data_sem));
> > +#endif
>
> We have lockdep_assert_not_held() for this so you can avoid the ifdef.
>
> > +
> > + while (ext4_test_inode_state(inode, EXT4_STATE_FC_COMMITTING)) {
> > +#if (BITS_PER_LONG < 64)
> > + DEFINE_WAIT_BIT(wait, &ei->i_state_flags,
> > + EXT4_STATE_FC_COMMITTING);
> > + wq = bit_waitqueue(&ei->i_state_flags,
> > + EXT4_STATE_FC_COMMITTING);
> > +#else
> > + DEFINE_WAIT_BIT(wait, &ei->i_flags,
> > + EXT4_STATE_FC_COMMITTING);
> > + wq = bit_waitqueue(&ei->i_flags,
> > + EXT4_STATE_FC_COMMITTING);
> > +#endif
> > + prepare_to_wait(wq, &wait.wq_entry, TASK_UNINTERRUPTIBLE);
> > + if (ext4_test_inode_state(inode, EXT4_STATE_FC_COMMITTING))
> > + schedule();
> > + finish_wait(wq, &wait.wq_entry);
> > + }
>
> But what protects us from fastcommit setting EXT4_STATE_FC_COMMITTING at
> this moment before we call ext4_fc_track_template(). Don't you need
> to grab sbi->s_fc_lock and hold it until the inode is attached to the
> fastcommit?
>
> I might be missing something so some documentation (like a comment here)
> would be nice to explain what are you actually trying to achieve with the
> waiting...
>
> Honza
> --
> Jan Kara <jack@xxxxxxxx>
> SUSE Labs, CR
