Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Content-Type: text/plain;
        charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3731.200.110.1.12\))
Subject: Re: [PATCH 2/6] Don't assume UID 0 attach
From:   evanhensbergen@icloud.com
In-Reply-To: <Y55Z2DwZgRG+9zW3@codewreck.org>
Date:   Sat, 17 Dec 2022 19:05:38 -0600
Cc:     v9fs-developer@lists.sourceforge.net,
        Ron Minnich <rminnich@gmail.com>,
        Latchesar Ionkov <lucho@ionkov.net>,
        linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
        linux_oss@crudebyte.com
Content-Transfer-Encoding: quoted-printable
Message-Id: <3343B7A9-2D1D-4A41-859E-B04AF90152FA@icloud.com>
References: <20221217185210.1431478-1-evanhensbergen@icloud.com>
 <20221217185210.1431478-3-evanhensbergen@icloud.com>
 <Y55Z2DwZgRG+9zW3@codewreck.org>
To:     asmadeus@codewreck.org
Precedence: bulk


> On Dec 17, 2022, at 6:07 PM, asmadeus@codewreck.org wrote:
>=20
> 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.
>=20
> 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
>=20

I=E2=80=99ll try and craft a test case for this, but I think it works?
That being said, I haven=E2=80=99t 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 =3D os.open("testfile", os.O_CREAT + =
os.O_RDWR, 0o400); os.write(f, b"ok\n")'
>=20
> 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=E2=80=99t run as root).  I need to go back and check, but if you =
are running as root and dftuid=3D0 then the behavior should be the same =
as before?
In any case, I=E2=80=99ll try to go back and make this work =E2=80=94 my =
big issue was always using uid 0 regardless of what mount options said =
is Wong.

>=20
>> 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.
>=20
> 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.
>=20
> Worth a try?

Yeah, that (using regular fids) is exactly what I am doing in my write =
back-fix patch which isn=E2=80=99t 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 =E2=80=94 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=E2=80=A6..

         -Eric