Received-SPF: pass (google.com: domain of linux-ext4+bounces-1848-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
From: Luis Henriques <luis.henriques@linux.dev>
To: Andreas Dilger <adilger@dilger.ca>
Cc: Theodore Ts'o <tytso@mit.edu>,  linux-ext4@vger.kernel.org
Subject: Re: [PATCH 3/4] tests: new test to check quota after directory
 optimization
In-Reply-To: <87il0zhms9.fsf@brahms.olymp> (Luis Henriques's message of "Tue,
	02 Apr 2024 15:17:10 +0100")
References: <20240328172940.1609-1-luis.henriques@linux.dev>
	<20240328172940.1609-4-luis.henriques@linux.dev>
	<D4FC7B67-F06B-493C-BCB7-29BD4A7D255F@dilger.ca>
	<87il0zhms9.fsf@brahms.olymp>
Date: Wed, 03 Apr 2024 16:03:49 +0100
Message-ID: <87r0fm5vze.fsf@brahms.olymp>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain

Luis Henriques <luis.henriques@linux.dev> writes:

> Andreas Dilger <adilger@dilger.ca> writes:
>
>> On Mar 28, 2024, at 11:29 AM, Luis Henriques (SUSE) <luis.henriques@linux.dev> wrote:
>>> 
>>> This new test validates e2fsck by verifying that quota data is updated
>>> after a directory optimization is performed.  It mimics fstest ext4/014
>>> by including a filesystem image where a file is created inside a new
>>> directory on the filesystem root and then root block 0 is wiped:
>>> 
>>>  # debugfs -w -R 'zap -f / 0' f_testnew/image
>>
>> I appreciate the test case, and I hate to be difficult, but IMHO this
>> test case is not ideal.  It is *still* reporting quota inconsistency
>> at the end, so it is difficult to see whether the patch is actually
>> improving anything or not?
>
> Maybe I misunderstood how the tests really work.  Here's what I
> understood:
>
> e2fsck is run twice.  During the first run the filesystem is recovered.
> And that's the output of expect.1 -- it reports the quota inconsistency
> because quota data needs to be fixed.  And it is fixed in that first run,
> where e2fsck returns '1' ("File system errors corrected").  The second
> time e2fsck is run (expect.2) it will do nothing, and '0' is returned
> because the filesystem hasn't been modified.
>
> Without the first patch in this series the second time e2fsck is executed
> it will still fail and report inconsistencies because the first time the
> fix wasn't correct.  (And after this second time the filesystem should
> actually be corrected, a third run of e2fsck should return '0'.)
>
>> This is because the image is testing a number of different things at
>> once (repairing the root inode, superblock, etc).  IMHO, it would be
>> better to have this test be specific to the directory shrink issue
>> (e.g. a large directory is created, many files are deleted from it,
>> then optimized), and ideally have a non-root user, group, and project
>> involved so that it is verifying that all of the quotas are updated.
>
> Right, that makes sense.  However, I'm failing to narrow the test to that
> specific case.  I've tried to create a bunch of files in a directory and
> used the debugfs 'kill_file' to remove files from that directory.
> However, in that case e2fsck isn't reporting quota inconsistencies as I
> would expect.  Which may hint at yet more quota-related bugs.  But I'm
> still looking.

OK, I _may_ have found a simple way to generate an image to test my patch.
Here's what I came up with:

    make testnew
    tune2fs -O quota f_testnew/image

    debugfs -w -R "ln lost+found foo" f_testnew/image
    debugfs -w -R "unlink lost+found" f_testnew/image

    echo "update quota on directory optimization" > f_testnew/name
    make testend
    mv f_testnew f_quota_shrinkdir

This will trigger a directory optimization after the recreation of the
lost+found directory.  Do you think this would be good enough?

Cheers,
-- 
Luis