From: Matthew Wilcox <willy@infradead.org>
To: David Sterba <dsterba@suse.cz>
Cc: Chris Mason <clm@fb.com>, Josef Bacik <josef@toxicpanda.com>,
David Sterba <dsterba@suse.com>,
linux-btrfs@vger.kernel.org, linux-fsdevel@vger.kernel.org,
Jan Kara <jack@suse.cz>
Subject: Re: Removing PG_error use from btrfs
Date: Thu, 18 Apr 2024 20:07:23 +0100 [thread overview]
Message-ID: <ZiFvawOnjZ7tDwPW@casper.infradead.org> (raw)
In-Reply-To: <20240418180051.GX3492@twin.jikos.cz>
On Thu, Apr 18, 2024 at 08:00:51PM +0200, David Sterba wrote:
> On Thu, Apr 18, 2024 at 06:41:47PM +0100, Matthew Wilcox wrote:
> > btrfs currently uses it to indicate superblock writeback errors.
> > This proposal moves that information to a counter in the btrfs_device.
> > Maybe this isn't the best approach. What do you think?
>
> Tracking the number of errors in the device is a good approach. The
> superblock write is asynchronous but it's not necessary to track the
> error in the page, we have the device structure in the end io callback.
> Also it's guaranteed that this is running only from one place so not
> even the atomics are needed.
Ah, but the completions might happen on different CPUs at the same time.
And the completion of the first write might happen while the submission
for a subsequent write are still running, and that might also adjust
the sb_wb_error value, so I think it is needed.
> I'd rather make the conversion from pages to folios a separate patch
> from the error counting change. I haven't seen anything obviously wrong
> but the superblock write is a critical action so it's a matter of
> precaution.
Understood. I've split off three patches for folio conversion; one for
each function.
> > - } else {
> > - SetPageUptodate(page);
> > + /* Ensure failure if a primary sb fails */
> > + if (bio->bi_opf & REQ_FUA)
> > + atomic_set(&device->sb_wb_errors, INT_MAX / 2);
>
> This is using some magic constant so it would be better defined
> separately and documented what it means.
I was hoping that comment would be enough. Name for the constant?
BTRFS_PRIMARY_SB_FAILED?
> Alternatively a flag can be set in the device if the primary superblock
> write fails but I think encoding that in the error count also works, as
> long as it's a named constant.
Yeah, I thought about that option too. This seemed like the easiest way
to go.
prev parent reply other threads:[~2024-04-18 19:07 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-18 17:41 Removing PG_error use from btrfs Matthew Wilcox
2024-04-18 18:00 ` David Sterba
2024-04-18 19:07 ` Matthew Wilcox [this message]
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=ZiFvawOnjZ7tDwPW@casper.infradead.org \
--to=willy@infradead.org \
--cc=clm@fb.com \
--cc=dsterba@suse.com \
--cc=dsterba@suse.cz \
--cc=jack@suse.cz \
--cc=josef@toxicpanda.com \
--cc=linux-btrfs@vger.kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
/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 read-only IMAP folder(s) and NNTP newsgroup(s).