Re: [RFC PATCH 2/2] libfs: Improve behavior when directory offset values wrap

[Chuck Lever <chuck.lever@oracle.com>]

On 11/22/24 3:49 AM, Amir Goldstein wrote:
> On Thu, Nov 21, 2024 at 10:30 PM Chuck Lever <chuck.lever@oracle.com> wrote:
>>
>> On Thu, Nov 21, 2024 at 01:18:05PM -0800, Hugh Dickins wrote:
>>> On Thu, 21 Nov 2024, Chuck Lever III wrote:
>>>>
>>>> I will note that tmpfs hangs during generic/449 for me 100%
>>>> of the time; the failure appears unrelated to renames. Do you
>>>> know if there is regular CI being done for tmpfs? I'm planning
>>>> to add it to my nightly test rig once I'm done here.
>>>
>>> For me generic/449 did not hang, just took a long time to discover
>>> something uninteresting and eventually declare "not run".  Took
>>> 14 minutes six years ago, when I gave up on it and short-circuited
>>> the "not run" with the patch below.
>>>
>>> (I carry about twenty patches for my own tmpfs fstests testing; but
>>> many of those are just for ancient 32-bit environment, or to suit the
>>> "huge=always" option. I never have enough time/priority to review and
>>> post them, but can send you a tarball if they might of use to you.)

I missed this offer first time around. Yes, please forward these.


>>> generic/449 is one of those tests which expects metadata to occupy
>>> space inside the "disk", in a way which it does not on tmpfs (and a
>>> quick glance at its history suggests btrfs also had issues with it).
>>>
>>> [PATCH] generic/449: not run on tmpfs earlier
>>>
>>> Do not waste 14 minutes to discover that tmpfs succeeds in
>>> setting acls despite running out of space for user attrs.
>>>
>>> Signed-off-by: Hugh Dickins <hughd@google.com>
>>> ---
>>>   tests/generic/449 | 5 +++++
>>>   1 file changed, 5 insertions(+)
>>>
>>> diff --git a/tests/generic/449 b/tests/generic/449
>>> index 9cf814ad..a52a992b 100755
>>> --- a/tests/generic/449
>>> +++ b/tests/generic/449
>>> @@ -22,6 +22,11 @@ _require_test
>>>   _require_acls
>>>   _require_attrs trusted
>>>
>>> +if [ "$FSTYP" = "tmpfs" ]; then
>>> +     # Do not waste 14 minutes to discover this:
>>> +     _notrun "$FSTYP succeeds in setting acls despite running out of space for user attrs"
>>> +fi
>>> +
>>>   _scratch_mkfs_sized $((256 * 1024 * 1024)) >> $seqres.full 2>&1
>>>   _scratch_mount || _fail "mount failed"
>>>
>>> --
>>> 2.35.3
>>
>> My approach (until I could look into the failure more) has been
>> similar:
>>
>> diff --git a/tests/generic/449 b/tests/generic/449
>> index 9cf814ad326c..8307a43ce87f 100755
>> --- a/tests/generic/449
>> +++ b/tests/generic/449
>> @@ -21,6 +21,7 @@ _require_scratch
>>   _require_test
>>   _require_acls
>>   _require_attrs trusted
>> +_supported_fs ^nfs ^overlay ^tmpfs
>>
> 
> nfs and overlay are _notrun because they do not support _scratch_mkfs_sized
> 
>>   _scratch_mkfs_sized $((256 * 1024 * 1024)) >> $seqres.full 2>&1
>>   _scratch_mount || _fail "mount failed"
>>
>>
>> I stole it from somewhere else, so it's not tmpfs-specific.
> 
> I think opt-out for a certain fs makes sense in some tests, but it is
> prefered to describe the requirement that is behind the opt-out.
> 
> For example, you thought that nfs,overlay,tmpfs should all opt-out
> from this test. Why? Which property do they share in common and
> how can it be described in a generic way?
> 
> I am not talking about a property that can be checked.
> Sometimes we need to make groups of filesystems that share a common
> property that cannot be tested, to better express the requirements.
> 
> _fstyp_has_non_default_seek_data_hole() is the only example that
> comes to mind but there could be others.
> 
> Thanks,
> Amir.

OK, I finally had a few minutes to look at this more closely.

As Hugh points out, tmpfs will permit users to set trusted xattrs
even when the file system is full. So IMO it should be excluded
from generic/449 explicitly -- this test won't produce meaningful
results. On my systems it runs for hours.

Hugh's patch above seems adequate. IMO that change or something
like it is entirely appropriate for upstream fstests.

NFS does not expose trusted xattrs on remote files.
"_require_attrs trusted" ought to block generic/449 already. I
will investigate/confirm that.

I'm happy to drop "^overlay". I don't have any explicit concern
about that file system type.


-- 
Chuck Lever