Re: [PATCH] btrfs: fix outstanding extents calculation

[Filipe Manana <fdmanana@kernel.org>]

On Mon, Mar 28, 2022 at 01:36:36PM +0000, Naohiro Aota wrote:
> On Mon, Mar 28, 2022 at 02:21:28PM +0100, Filipe Manana wrote:
> > On Mon, Mar 28, 2022 at 01:14:02PM +0000, Johannes Thumshirn wrote:
> > > On 28/03/2022 15:09, Filipe Manana wrote:
> > > > On Mon, Mar 28, 2022 at 09:32:05PM +0900, Naohiro Aota wrote:
> > > >> Running generic/406 causes the following WARNING in btrfs_destroy_inode()
> > > >> which tells there is outstanding extents left.
> > > > 
> > > > I can't trigger the warning with generic/406.
> > > > Any special setup or config to trigger it?
> > > > 
> > > > The change looks fine to me, however I'm curious why this isn't triggered
> > > > with generic/406, nor anyone else reported it before, since the test is
> > > > fully deterministic.
> > > > 
> > > 
> > > I am able to trigger the WARN() with a different test (which is for a different,
> > > not yet solved problem) on my zoned setup. With this patch, the WARN() is gone.
> > 
> > I have no doubts about the fix being correct.
> > I'm just puzzled why I can't trigger it with generic/406, given that it's a very
> > deterministic test.
> >
> > If there's any special config or setup (mount options, zoned fs, etc), I would
> > like to have it explicitly mentioned in the changelog.
> 
> I don't think it's super special. I can always reproduce it on 1GB
> zram device. Here is the mkfs setup, and no mount options are
> specified.

Ok, that, the 1G device, explains it.

It's trigfering a short-write, due to not being able to allocate a large extent and
instead allocating a smaller one. And the test does not fail if we write less than
what we requested, as it redirects xfs_io's stdout to the .full file and does not
check that we wrote the exact amount we asked to write (which is a rather bad test
IMO, should also check we are able to read what we wrote before, etc).

The change looks good to me.

With an updated subject, you can add:

Reviewed-by: Filipe Manana <fdmanana@suse.com>

Thanks.

> 
> ++ mkfs.btrfs -f -d single -m single /dev/zram0
> btrfs-progs v5.16.2
> See http://btrfs.wiki.kernel.org for more information.
> 
> Performing full device TRIM /dev/zram0 (1.00GiB) ...
> NOTE: several default settings have changed in version 5.15, please make sure
>       this does not affect your deployments:
>       - DUP for metadata (-m dup)
>       - enabled no-holes (-O no-holes)
>       - enabled free-space-tree (-R free-space-tree)
> 
> Label:              (null)
> UUID:               b7260fb1-fa0e-4acd-8c3d-0530799a9fd3
> Node size:          16384
> Sector size:        4096
> Filesystem size:    1.00GiB
> Block group profiles:
>   Data:             single            8.00MiB
>   Metadata:         single            8.00MiB
>   System:           single            4.00MiB
> SSD detected:       yes
> Zoned device:       no
> Incompat features:  extref, skinny-metadata, no-holes
> Runtime features:   free-space-tree
> Checksum:           crc32c
> Number of devices:  1
> Devices:
>    ID        SIZE  PATH
>     1     1.00GiB  /dev/zram0