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:
> @@ -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
