From: Boris Burkov <boris@bur.io>
To: Martin Raiber <martin@urbackup.org>
Cc: Josef Bacik <josef@toxicpanda.com>,
Qu Wenruo <quwenruo.btrfs@gmx.com>, Chris Mason <clm@fb.com>,
David Sterba <dsterba@suse.com>,
linux-btrfs@vger.kernel.org, kernel-team@fb.com
Subject: Re: [PATCH RFC] btrfs: change commit txn to end txn in subvol_setflags ioctl
Date: Fri, 7 Aug 2020 13:45:33 -0700 [thread overview]
Message-ID: <20200807204533.GA429307@devvm842.ftw2.facebook.com> (raw)
In-Reply-To: <01020173bed9de46-08b668c9-e188-412b-846c-4ed33ce2fe4f-000000@eu-west-1.amazonses.com>
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.
next prev parent reply other threads:[~2020-08-07 20:45 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-08-04 17:55 [PATCH RFC] btrfs: change commit txn to end txn in subvol_setflags ioctl Boris Burkov
2020-08-04 22:48 ` Qu Wenruo
2020-08-04 23:08 ` Josef Bacik
2020-08-05 13:40 ` Martin Raiber
2020-08-07 20:45 ` Boris Burkov [this message]
2020-08-10 18:05 ` Martin Raiber
2020-08-25 20:23 ` Boris Burkov
2020-08-26 14:23 ` Josef Bacik
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200807204533.GA429307@devvm842.ftw2.facebook.com \
--to=boris@bur.io \
--cc=clm@fb.com \
--cc=dsterba@suse.com \
--cc=josef@toxicpanda.com \
--cc=kernel-team@fb.com \
--cc=linux-btrfs@vger.kernel.org \
--cc=martin@urbackup.org \
--cc=quwenruo.btrfs@gmx.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).