Re: [PATCH] btrfs: run delayed iput at unlink time

[David Sterba <dsterba@xxxxxxx>]

On Wed, May 08, 2019 at 10:15:16AM +0300, Nikolay Borisov wrote:
> > +	if (!list_empty(&inode->delayed_iput)) {
> > +		spin_lock(&fs_info->delayed_iput_lock);
> > +		if (!list_empty(&inode->delayed_iput)) {
> > +			list_del_init(&inode->delayed_iput);
> > +			spin_unlock(&fs_info->delayed_iput_lock);
> > +			iput(&inode->vfs_inode);
> > +			if (atomic_dec_and_test(&fs_info->nr_delayed_iputs))
> > +				wake_up(&fs_info->delayed_iputs_wait);
> > +		} else {
> > +			spin_unlock(&fs_info->delayed_iput_lock);
> > +		}
> > +	}
> 
> OTOH this really feels like a hack and this stems from the fact that
> iput is rather rudimentary. Additionally you are essentially opencoding
> the body of btrfs_run_delayed_iputs. I was going to suggest to introduce
> a new helper factoring out the common code but it will get ugly due to
> the spin lock being dropped before doing the iput.

Yeah this should be in a helper, something like

--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -3286,6 +3286,26 @@ void btrfs_add_delayed_iput(struct inode *inode)
                wake_up_process(fs_info->cleaner_kthread);
 }
 
+static void run_delayed_iput_now(struct inode *inode)
+{
+       list_del_init(&inode->delayed_iput);
+       spin_unlock(&fs_info->delayed_iput_lock);
+       iput(&inode->vfs_inode);
+       if (atomic_dec_and_test(&fs_info->nr_delayed_iputs))
+               wake_up(&fs_info->delayed_iputs_wait);
+       spin_lock(&fs_info->delayed_iput_lock);
+}
+
+void btrfs_run_delayed_iput_now(struct inode *inode)
+{
+       spin_lock(&fs_info->delayed_iput_lock);
+       if (list_empty(&inode->delayed_iput))
+               goto out_unlock;
+       run_delayed_iput_now(inode);
+out_unlock:
+       unspin_lock(&fs_info->delayed_iput_lock);
+}
+
 void btrfs_run_delayed_iputs(struct btrfs_fs_info *fs_info)
 {
 
@@ -3295,12 +3315,8 @@ void btrfs_run_delayed_iputs(struct btrfs_fs_info *fs_info)
 
                inode = list_first_entry(&fs_info->delayed_iputs,
                                struct btrfs_inode, delayed_iput);
-               list_del_init(&inode->delayed_iput);
-               spin_unlock(&fs_info->delayed_iput_lock);
-               iput(&inode->vfs_inode);
-               if (atomic_dec_and_test(&fs_info->nr_delayed_iputs))
-                       wake_up(&fs_info->delayed_iputs_wait);
-               spin_lock(&fs_info->delayed_iput_lock);
+
+               run_delayed_iput_now(inode);
        }
        spin_unlock(&fs_info->delayed_iput_lock);
 }
---

The delayed_iput_lock is not that contended so that the first check needs to be
done unlocked. There are only list manipulations in the critical section.

The above does one unnecessary lock/unlock in case the standalone
delayed iput is called, I don't see a cleaner way now.