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.
