Re: [PATCH 3/4] btrfs: autodefrag: only scan one inode once

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2022/2/23 01:32, David Sterba wrote:
> On Sun, Feb 13, 2022 at 03:42:32PM +0800, Qu Wenruo wrote:
>> Although we have btrfs_requeue_inode_defrag(), for autodefrag we are
>> still just exhausting all inode_defrag items in the tree.
>>
>> This means, it doesn't make much difference to requeue an inode_defrag,
>> other than scan the inode from the beginning till its end.
>>
>> This patch will change the beahvior by always scan from offset 0 of an
>> inode, and till the end of the inode.
>>
>> By this we get the following benefit:
>>
>> - Straight-forward code
>>
>> - No more re-queue related check
>>
>> - Less members in inode_defrag
>>
>> We still keep the same btrfs_get_fs_root() and btrfs_iget() check for
>> each loop, and added extra should_auto_defrag() check per-loop.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>
> Below is a version of the patch without the control structure and with a
> manual while (true) loop so there's not that much code moved and it's
> clear what's being added. I haven't tested it yet, but this is what I'd
> like to get merged and then forwarded to stable so we can finally get
> over this.
>
>   fs/btrfs/file.c | 84 +++++++++++++------------------------------------
>   1 file changed, 22 insertions(+), 62 deletions(-)
>
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index 62c4edd5e2f9..1efc378e4bbe 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -49,12 +49,6 @@ struct inode_defrag {
>
>   	/* root objectid */
>   	u64 root;
> -
> -	/* last offset we were able to defrag */
> -	u64 last_offset;
> -
> -	/* if we've wrapped around back to zero once already */
> -	int cycled;
>   };
>
>   static int __compare_inode_defrag(struct inode_defrag *defrag1,
> @@ -107,8 +101,6 @@ static int __btrfs_add_inode_defrag(struct btrfs_inode *inode,
>   			 */
>   			if (defrag->transid < entry->transid)
>   				entry->transid = defrag->transid;
> -			if (defrag->last_offset > entry->last_offset)
> -				entry->last_offset = defrag->last_offset;
>   			return -EEXIST;
>   		}
>   	}
> @@ -178,34 +170,6 @@ int btrfs_add_inode_defrag(struct btrfs_trans_handle *trans,
>   	return 0;
>   }
>
> -/*
> - * Requeue the defrag object. If there is a defrag object that points to
> - * the same inode in the tree, we will merge them together (by
> - * __btrfs_add_inode_defrag()) and free the one that we want to requeue.
> - */
> -static void btrfs_requeue_inode_defrag(struct btrfs_inode *inode,
> -				       struct inode_defrag *defrag)
> -{
> -	struct btrfs_fs_info *fs_info = inode->root->fs_info;
> -	int ret;
> -
> -	if (!__need_auto_defrag(fs_info))
> -		goto out;
> -
> -	/*
> -	 * Here we don't check the IN_DEFRAG flag, because we need merge
> -	 * them together.
> -	 */
> -	spin_lock(&fs_info->defrag_inodes_lock);
> -	ret = __btrfs_add_inode_defrag(inode, defrag);
> -	spin_unlock(&fs_info->defrag_inodes_lock);
> -	if (ret)
> -		goto out;
> -	return;
> -out:
> -	kmem_cache_free(btrfs_inode_defrag_cachep, defrag);
> -}
> -
>   /*
>    * pick the defragable inode that we want, if it doesn't exist, we will get
>    * the next one.
> @@ -278,8 +242,14 @@ static int __btrfs_run_defrag_inode(struct btrfs_fs_info *fs_info,
>   	struct btrfs_root *inode_root;
>   	struct inode *inode;
>   	struct btrfs_ioctl_defrag_range_args range;
> -	int num_defrag;
> -	int ret;
> +	int ret = 0;
> +	u64 cur = 0;
> +
> +again:
> +	if (test_bit(BTRFS_FS_STATE_REMOUNTING, &fs_info->fs_state))
> +		goto cleanup;
> +	if (!__need_auto_defrag(fs_info))
> +		goto cleanup;
>
>   	/* get the inode */
>   	inode_root = btrfs_get_fs_root(fs_info, defrag->root, true);
> @@ -295,39 +265,29 @@ static int __btrfs_run_defrag_inode(struct btrfs_fs_info *fs_info,
>   		goto cleanup;
>   	}
>
> +	if (cur >= i_size_read(inode)) {
> +		iput(inode);
> +		break;

Would this even compile?
Break without a while loop?

To me, the open-coded while loop using goto is even worse.
I don't think just saving one indent is worthy.

Where can I find the final version to do more testing/review?

Thanks,
Qu

> +	}
> +
>   	/* do a chunk of defrag */
>   	clear_bit(BTRFS_INODE_IN_DEFRAG, &BTRFS_I(inode)->runtime_flags);
>   	memset(&range, 0, sizeof(range));
>   	range.len = (u64)-1;
> -	range.start = defrag->last_offset;
> +	range.start = cur;
>
>   	sb_start_write(fs_info->sb);
> -	num_defrag = btrfs_defrag_file(inode, NULL, &range, defrag->transid,
> +	ret = btrfs_defrag_file(inode, NULL, &range, defrag->transid,
>   				       BTRFS_DEFRAG_BATCH);
>   	sb_end_write(fs_info->sb);
> -	/*
> -	 * if we filled the whole defrag batch, there
> -	 * must be more work to do.  Queue this defrag
> -	 * again
> -	 */
> -	if (num_defrag == BTRFS_DEFRAG_BATCH) {
> -		defrag->last_offset = range.start;
> -		btrfs_requeue_inode_defrag(BTRFS_I(inode), defrag);
> -	} else if (defrag->last_offset && !defrag->cycled) {
> -		/*
> -		 * we didn't fill our defrag batch, but
> -		 * we didn't start at zero.  Make sure we loop
> -		 * around to the start of the file.
> -		 */
> -		defrag->last_offset = 0;
> -		defrag->cycled = 1;
> -		btrfs_requeue_inode_defrag(BTRFS_I(inode), defrag);
> -	} else {
> -		kmem_cache_free(btrfs_inode_defrag_cachep, defrag);
> -	}
> -
>   	iput(inode);
> -	return 0;
> +
> +	if (ret < 0)
> +		goto cleanup;
> +
> +	cur = max(cur + fs_info->sectorsize, range.start);
> +	goto again;
> +
>   cleanup:
>   	kmem_cache_free(btrfs_inode_defrag_cachep, defrag);
>   	return ret;