Re: [PATCH 2/6] Don't assume UID 0 attach

[evanhensbergen]

> On Dec 17, 2022, at 6:07 PM, asmadeus@xxxxxxxxxxxxx wrote:
> 
> Eric Van Hensbergen wrote on Sat, Dec 17, 2022 at 06:52:06PM +0000:
>> The writeback_fid fallback code assumes a root uid fallback which
>> breaks many server configurations (which don't run as root).  This
>> patch switches to generic lookup which will follow argument
>> guidence on access modes and default ids to use on failure.
> 
> Unfortunately this one will break writes to a file created as 400 I
> think
> That's the main reason we have this writeback fid afaik -- there are
> cases where the user should be able to write to the file, but a plain
> open/write won't work... I can't think of anything else than open 400
> right now though
> 

I’ll try and craft a test case for this, but I think it works?
That being said, I haven’t been trying the xfstests, just fsx and bench.

> I'm sure there's an xfs_io command and xfstest for that, but for now:
> python3 -c 'import os; f = os.open("testfile", os.O_CREAT + os.O_RDWR, 0o400); os.write(f, b"ok\n")'
> 
> iirc ganesha running as non-root just ignores root requests and opens as
> current user-- this won't work for this particular case, but might be
> good enough for you... With that said:

Yeah, the real problem I ran into this was if the server is running as non-root this causes issues and I was testing against cpu (which doesn’t run as root).  I need to go back and check, but if you are running as root and dftuid=0 then the behavior should be the same as before?
In any case, I’ll try to go back and make this work — my big issue was always using uid 0 regardless of what mount options said is Wong.

> 
>> There is a deeper underlying problem with writeback_fids in that
>> this fallback is too standard and not an exception due to the way
>> writeback mode works in the current implementation.  Subsequent
>> patches will try to associate writeback fids from the original user
>> either by flushing on close or by holding onto fid until writeback
>> completes.
> 
> If we can address this problem though I agree we should stop using
> wrieback fids as much as we do.
> Now fids are refcounted, I think we could just use the normal fid as
> writeback fid (getting a ref), and the regular close will not clunk it
> so delayed IOs will pass.
> 
> Worth a try?

Yeah, that (using regular fids) is exactly what I am doing in my write back-fix patch which isn’t part of this series.  I was still hunting a few bugs, but I think I nailed them today.  I have to do a more extensive test sweep of the different configs, but unit tests seem good to go now so if I end up reworking the patch set to address your comment above, I may just go ahead and add it to the resubmit set.  However, I also go ahead and flush on close/clunk — and that gets rid of the delayed write back which I think is actually preferable anyways.  I may re-introduce it with temporal caching, but its just so problematic…..

         -Eric