Test generic/614

Test generic/614

[Jan Kara]

Hello Filipe!

I have noticed test generic/614 is failing on ext2. The test creates sparse
file, maps it, and writes data to it through mmap. Then it checks i_blocks
has increased. However for ext2 (and other trivial filesystems supporting
sparse files but not implementing delayed allocation) this is bound to fail
because i_block gets incremented only during page writeback.

So would your btrfs test make sense even if we called fsync(2) before
checking stat(2) results?

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: Test generic/614

[Filipe Manana]

On Mon, Nov 21, 2022 at 12:37 PM Jan Kara <jack@suse.cz> wrote:
>
> Hello Filipe!

Hi Jan!

>
> I have noticed test generic/614 is failing on ext2. The test creates sparse
> file, maps it, and writes data to it through mmap. Then it checks i_blocks
> has increased. However for ext2 (and other trivial filesystems supporting
> sparse files but not implementing delayed allocation) this is bound to fail
> because i_block gets incremented only during page writeback.
>
> So would your btrfs test make sense even if we called fsync(2) before
> checking stat(2) results?

Calling fsync before calling stat would defeat the purpose of the test.
I.e. the test would pass both with and without the btrfs fix.

So maybe adding the following to the test:

_supported_fs ^ext2

And listing all other filesystems not supporting delayed allocation?

Or adding a _require_delayed_allocation() helper to make it _notrun().

Thanks.



>
>                                                                 Honza
> --
> Jan Kara <jack@suse.com>
> SUSE Labs, CR

Re: Test generic/614

[Jan Kara]

On Mon 21-11-22 13:32:22, Filipe Manana wrote:
> On Mon, Nov 21, 2022 at 12:37 PM Jan Kara <jack@suse.cz> wrote:
> >
> > Hello Filipe!
> 
> Hi Jan!
> 
> >
> > I have noticed test generic/614 is failing on ext2. The test creates sparse
> > file, maps it, and writes data to it through mmap. Then it checks i_blocks
> > has increased. However for ext2 (and other trivial filesystems supporting
> > sparse files but not implementing delayed allocation) this is bound to fail
> > because i_block gets incremented only during page writeback.
> >
> > So would your btrfs test make sense even if we called fsync(2) before
> > checking stat(2) results?
> 
> Calling fsync before calling stat would defeat the purpose of the test.
> I.e. the test would pass both with and without the btrfs fix.
> 
> So maybe adding the following to the test:
> 
> _supported_fs ^ext2
> 
> And listing all other filesystems not supporting delayed allocation?

Yeah, probably something like this. In fact, it is a filesystem bug that we
don't reserve space for blocks written through mmap but one that is
basically by design and difficult to fix. I'll think how to best avoid this
failure.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: Test generic/614

[Darrick J. Wong]

On Mon, Nov 21, 2022 at 01:32:22PM +0000, Filipe Manana wrote:
> On Mon, Nov 21, 2022 at 12:37 PM Jan Kara <jack@suse.cz> wrote:
> >
> > Hello Filipe!
> 
> Hi Jan!
> 
> >
> > I have noticed test generic/614 is failing on ext2. The test creates sparse
> > file, maps it, and writes data to it through mmap. Then it checks i_blocks
> > has increased. However for ext2 (and other trivial filesystems supporting
> > sparse files but not implementing delayed allocation) this is bound to fail
> > because i_block gets incremented only during page writeback.
> >
> > So would your btrfs test make sense even if we called fsync(2) before
> > checking stat(2) results?
> 
> Calling fsync before calling stat would defeat the purpose of the test.
> I.e. the test would pass both with and without the btrfs fix.
> 
> So maybe adding the following to the test:
> 
> _supported_fs ^ext2
> 
> And listing all other filesystems not supporting delayed allocation?
> 
> Or adding a _require_delayed_allocation() helper to make it _notrun().

_require_scratch_delalloc() ?

--D

> Thanks.
> 
> 
> 
> >
> >                                                                 Honza
> > --
> > Jan Kara <jack@suse.com>
> > SUSE Labs, CR

Re: Test generic/614

[Jan Kara]

On Mon 21-11-22 08:51:43, Darrick J. Wong wrote:
> On Mon, Nov 21, 2022 at 01:32:22PM +0000, Filipe Manana wrote:
> > On Mon, Nov 21, 2022 at 12:37 PM Jan Kara <jack@suse.cz> wrote:
> > >
> > > Hello Filipe!
> > 
> > Hi Jan!
> > 
> > >
> > > I have noticed test generic/614 is failing on ext2. The test creates sparse
> > > file, maps it, and writes data to it through mmap. Then it checks i_blocks
> > > has increased. However for ext2 (and other trivial filesystems supporting
> > > sparse files but not implementing delayed allocation) this is bound to fail
> > > because i_block gets incremented only during page writeback.
> > >
> > > So would your btrfs test make sense even if we called fsync(2) before
> > > checking stat(2) results?
> > 
> > Calling fsync before calling stat would defeat the purpose of the test.
> > I.e. the test would pass both with and without the btrfs fix.
> > 
> > So maybe adding the following to the test:
> > 
> > _supported_fs ^ext2
> > 
> > And listing all other filesystems not supporting delayed allocation?
> > 
> > Or adding a _require_delayed_allocation() helper to make it _notrun().
> 
> _require_scratch_delalloc() ?

Yeah, after some experimentation I think I can even sensibly detect
delalloc using filefrag(1) (xfs_io's fiemap command always syncs file
before doing fiemap). I'll give it a shot.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: Test generic/614

[Jan Kara]

On Tue 22-11-22 13:52:41, Jan Kara wrote:
> On Mon 21-11-22 08:51:43, Darrick J. Wong wrote:
> > On Mon, Nov 21, 2022 at 01:32:22PM +0000, Filipe Manana wrote:
> > > On Mon, Nov 21, 2022 at 12:37 PM Jan Kara <jack@suse.cz> wrote:
> > > >
> > > > Hello Filipe!
> > > 
> > > Hi Jan!
> > > 
> > > >
> > > > I have noticed test generic/614 is failing on ext2. The test creates sparse
> > > > file, maps it, and writes data to it through mmap. Then it checks i_blocks
> > > > has increased. However for ext2 (and other trivial filesystems supporting
> > > > sparse files but not implementing delayed allocation) this is bound to fail
> > > > because i_block gets incremented only during page writeback.
> > > >
> > > > So would your btrfs test make sense even if we called fsync(2) before
> > > > checking stat(2) results?
> > > 
> > > Calling fsync before calling stat would defeat the purpose of the test.
> > > I.e. the test would pass both with and without the btrfs fix.
> > > 
> > > So maybe adding the following to the test:
> > > 
> > > _supported_fs ^ext2
> > > 
> > > And listing all other filesystems not supporting delayed allocation?
> > > 
> > > Or adding a _require_delayed_allocation() helper to make it _notrun().
> > 
> > _require_scratch_delalloc() ?
> 
> Yeah, after some experimentation I think I can even sensibly detect
> delalloc using filefrag(1) (xfs_io's fiemap command always syncs file
> before doing fiemap). I'll give it a shot.

Aha, _require_scratch_delalloc already exists ;)

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR