Received-SPF: pass (google.com: domain of linux-ext4+bounces-1824-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
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: <D4FC7B67-F06B-493C-BCB7-29BD4A7D255F@dilger.ca> (Andreas
	Dilger's message of "Mon, 1 Apr 2024 15:01:33 -0600")
References: <20240328172940.1609-1-luis.henriques@linux.dev>
	<20240328172940.1609-4-luis.henriques@linux.dev>
	<D4FC7B67-F06B-493C-BCB7-29BD4A7D255F@dilger.ca>
Date: Tue, 02 Apr 2024 15:17:10 +0100
Message-ID: <87il0zhms9.fsf@brahms.olymp>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain

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.

Cheers,
-- 
Luis