Re: [PATCH RFC] btrfs: change commit txn to end txn in subvol_setflags ioctl

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

[-- Attachment #1.1: Type: text/plain, Size: 2527 bytes --]



On 2020/8/5 上午1:55, Boris Burkov wrote:
> Currently, btrfs_ioctl_subvol_setflags forces a btrfs_commit_transaction
> while holding subvol_sem. As a result, we have seen workloads where
> calling `btrfs property set -ts <subvol> ro false` hangs waiting for a
> legitimately slow commit. This gets even worse if the workload tries to
> set flags on multiple subvolumes and the ioctls pile up on subvol_sem.
> 
> Change the commit to a btrfs_end_transaction so that the ioctl can
> return in a timely fashion and piggy back on a later commit.
> 
> Signed-off-by: Boris Burkov <boris@bur.io>
> ---
>  fs/btrfs/ioctl.c       | 2 +-
>  fs/btrfs/transaction.c | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index bd3511c5ca81..3ae484768ce7 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -1985,7 +1985,7 @@ static noinline int btrfs_ioctl_subvol_setflags(struct file *file,
>  		goto out_reset;
>  	}
>  
> -	ret = btrfs_commit_transaction(trans);
> +	ret = btrfs_end_transaction(trans);

This means the setflag is not committed to disk, and if a powerloss
happens before a transaction commit, then the setflag operation just get
lost.

This means, previously if this ioctl returns, users can expect that the
flag is always set no matter what, but now there is no guarantee.

Personally I'm not sure if we really want that operation to be committed
to disk.
Maybe that transaction commit can be initialized in user space, so for
multiple setflags, we only commit once, thus saves a lot of time.

Thanks,
Qu

>  
>  out_reset:
>  	if (ret)
> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> index 20c6ac1a5de7..1dc44209c2ae 100644
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -47,7 +47,7 @@
>   * | Will wait for previous running transaction to completely finish if there
>   * | is one
>   * |
> - * | Then one of the following happes:
> + * | Then one of the following happens:
>   * | - Wait for all other trans handle holders to release.
>   * |   The btrfs_commit_transaction() caller will do the commit work.
>   * | - Wait for current transaction to be committed by others.
> @@ -60,7 +60,7 @@
>   * |
>   * | To next stage:
>   * |  Caller is chosen to commit transaction N, and all other trans handle
> - * |  haven been released.
> + * |  have been released.
>   * V
>   * Transaction N [[TRANS_STATE_COMMIT_DOING]]
>   * |
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]