Re: [PATCH] generic: test Btrfs delalloc accounting overflow

Re: [PATCH] generic: test Btrfs delalloc accounting overflow

[Eryu Guan]

On Fri, Jun 02, 2017 at 02:46:52PM +0200, David Sterba wrote:
> On Fri, Jun 02, 2017 at 12:07:37PM +0300, Nikolay Borisov wrote:
> > > +# Make sure that we didn't leak any metadata space.
> > > +if [[ $FSTYP = btrfs ]]; then
> > > +	uuid="$(findmnt -n -o UUID "$TEST_DIR")"
> > 
> > if we are on btrfs and we don't have findmnt this test will likely fail.
> > Perhaps include a _require_command findmnt
> 
> I think utilities like findmnt should be checked at the beginning of the
> whole testuiste, not in each test that uses them. As findmnt is part of

Agreed. I think we can define a FINDMNT_PROG in common/config and refuse
to run any test if it's mising, as what we did to $MOUNT_PROG and other
must-have commands. (There's already a bare call to findmnt in
common/rc, change it to call $FINDMNT_PROG too).

We can do this in a separate patch.

Thanks,
Eryu

> util-linux, missing it would also mean that eg 'mount' is missing.
> Highly unlikely.
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] generic: test Btrfs delalloc accounting overflow

[Christoph Hellwig]

This looks like a btrfs-specific test, and not like a generic one
to me.

Re: [PATCH] generic: test Btrfs delalloc accounting overflow

[Omar Sandoval]

On Sat, Jun 03, 2017 at 12:37:00AM -0700, Christoph Hellwig wrote:
> This looks like a btrfs-specific test, and not like a generic one
> to me.

Nothing about the workload itself is btrfs-specific, we just have the
extra check at the end. But I don't really care, I can make it a btrfs
test unless Eryu has already applied it.

Re: [PATCH] generic: test Btrfs delalloc accounting overflow

[Eryu Guan]

On Tue, Jun 06, 2017 at 05:03:05PM -0700, Omar Sandoval wrote:
> On Sat, Jun 03, 2017 at 12:37:00AM -0700, Christoph Hellwig wrote:
> > This looks like a btrfs-specific test, and not like a generic one
> > to me.
> 
> Nothing about the workload itself is btrfs-specific, we just have the
> extra check at the end. But I don't really care, I can make it a btrfs
> test unless Eryu has already applied it.

What's not very clear to me is that how do we check the accountings for
non-btrfs filesystems, i.e. what would cause a test failure on non-btrfs
filesystems besides an xfs_io write failure? I'd prefer a btrfs-specific
test if there's no good way to do the check. Or if we want to keep it a
generic test, some comments on the non-btrfs case would be good. I'm
fine with either way.

Thanks,
Eryu

Re: [PATCH] generic: test Btrfs delalloc accounting overflow

[Omar Sandoval]

On Wed, Jun 07, 2017 at 11:08:01AM +0800, Eryu Guan wrote:
> On Tue, Jun 06, 2017 at 05:03:05PM -0700, Omar Sandoval wrote:
> > On Sat, Jun 03, 2017 at 12:37:00AM -0700, Christoph Hellwig wrote:
> > > This looks like a btrfs-specific test, and not like a generic one
> > > to me.
> > 
> > Nothing about the workload itself is btrfs-specific, we just have the
> > extra check at the end. But I don't really care, I can make it a btrfs
> > test unless Eryu has already applied it.
> 
> What's not very clear to me is that how do we check the accountings for
> non-btrfs filesystems, i.e. what would cause a test failure on non-btrfs
> filesystems besides an xfs_io write failure? I'd prefer a btrfs-specific
> test if there's no good way to do the check. Or if we want to keep it a
> generic test, some comments on the non-btrfs case would be good. I'm
> fine with either way.
> 
> Thanks,
> Eryu

The only thing I can imagine other filesystems hitting would be an early
enospc or some sort of kernel warning, so I'll resend it as a btrfs test.