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

[Boris Burkov <boris@bur.io>]

On Wed, Aug 05, 2020 at 01:40:16PM +0000, Martin Raiber wrote:
> On 05.08.2020 01:08 Josef Bacik wrote:
> >On 8/4/20 6:48 PM, Qu Wenruo wrote:
> >>
> >>
> >>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.
> >>
> >
> >I'm of the opinion that we shouldn't be committing the transaction
> >for stuff like this, unless there's a really good reason to.
> >Especially given we're holding the subvol lock here, we should
> >just do end_transaction.  Thanks,
> From a user perspective I'd appreciate having the option to set it
> in a non-durable way (I have seen btrfs property sets hanging for a
> long time as well). But currently my application kind of depends on
> it being durable. Making it non-durable wouldn't break much and I
> guess the old behaviour could be emulated by a "btrfs fi sync
> <subvol>" afterwards, but idk how much other stuff depends on it
> being durable. Making it consistent with btrfs subvol del with the
> "-c" switch would be nice and consistent as well (and the -c switch
> could be done via IOC_SYNC after setting the properties).

Martin,

Thanks for your perspective, that's helpful. Could you elaborate on how
your application relies on the durability? I would just like to learn
more about how this might affect people.

I really like the -c idea, but I fear if people are broadly depending on
that behavior by default, it wouldn't be enough.