On Sun 18-08-24 04:03:56, Harshad Shirwadkar wrote:
> Leaving s_fc_lock in between during commit in ext4_fc_perform_commit()
> function leaves room for subtle concurrency bugs where ext4_fc_del() may
> delete an inode from the fast commit list, leaving list in an inconsistent
> state. Also, this patch converts s_fc_lock to mutex type so that it can be
> held when kmem_cache_* functions are called.
>
> Signed-off-by: Harshad Shirwadkar <harshadshirwadkar@xxxxxxxxx>
This would be easier to review if you split the patch into two - one for
mindless conversion of a spinlock into a mutex and another one for the
change when the lock is held which can have non-trivial effects.
Otherwise the patch looks good, just one nit below. With that fixed feel
free to add:
Reviewed-by: Jan Kara <jack@xxxxxxx>
> @@ -1010,26 +1011,22 @@ __releases(&sbi->s_fc_lock)
^^ the sparse annotation with __acquires, __releases needs updating as
well
> list_for_each_entry_safe(fc_dentry, fc_dentry_n,
> &sbi->s_fc_dentry_q[FC_Q_MAIN], fcd_list) {
> if (fc_dentry->fcd_op != EXT4_FC_TAG_CREAT) {
> - spin_unlock(&sbi->s_fc_lock);
> - if (!ext4_fc_add_dentry_tlv(sb, crc, fc_dentry)) {
> - ret = -ENOSPC;
> - goto lock_and_exit;
> - }
> - spin_lock(&sbi->s_fc_lock);
> + if (!ext4_fc_add_dentry_tlv(sb, crc, fc_dentry))
> + return -ENOSPC;
> continue;
> }
> /*
> * With fcd_dilist we need not loop in sbi->s_fc_q to get the
> - * corresponding inode pointer
> + * corresponding inode. Also, the corresponding inode could have been
> + * deleted, in which case, we don't need to do anything.
> */
> - WARN_ON(list_empty(&fc_dentry->fcd_dilist));
> + if (list_empty(&fc_dentry->fcd_dilist))
> + continue;
> ei = list_first_entry(&fc_dentry->fcd_dilist,
> struct ext4_inode_info, i_fc_dilist);
> inode = &ei->vfs_inode;
> WARN_ON(inode->i_ino != fc_dentry->fcd_ino);
>
> - spin_unlock(&sbi->s_fc_lock);
> -
> /*
> * We first write the inode and then the create dirent. This
> * allows the recovery code to create an unnamed inode first
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
