Re: [RFC PATCH v6 00/20] ceph+fscrypt: context, filename and symlink support

[Luis Henriques <lhenriques@suse.de>]

Jeff Layton <jlayton@kernel.org> writes:

> On Mon, 2021-04-19 at 17:03 +0100, Luis Henriques wrote:
>> Jeff Layton <jlayton@kernel.org> writes:
>> 
>> > On Mon, 2021-04-19 at 11:30 +0100, Luis Henriques wrote:
>> ...
>> > Ouch. That looks like a real bug, alright.
>> > 
>> > Basically when building the path, we occasionally need to fetch the
>> > crypto context for parent inodes and such, and that can cause us to
>> > recurse back into __ceph_getxattr and try to issue another RPC to the
>> > MDS.
>> > 
>> > I'll have to look and see what we can do. Maybe it's safe to drop the
>> > mdsc->mutex while we're building the path? Or maybe this is a good time
>> > to re-think a lot of the really onerous locking in this codepath?
>> > 
>> > I'm open to suggestions here...
>> 
>> Yeah, I couldn't see a good fix at a first glace.  Dropping the mutex
>> while building the path was my initial thought too but it's not easy to
>> proof that's a safe thing to do.
>> 
>
> Indeed. It's an extremely coarse-grained mutex and not at all clear what
> it protects here.
>
>> The other idea I had was to fetch all the needed fscrypt contexts at the
>> end, after building the path.  But I didn't found a way for doing that
>> because to build the path... we need the contexts.
>> 
>> It looks like this leaves us with the locking rethinking option.
>> 
>> /me tries harder to find another way out
>> 
>> Cheers,
>
> The other option I think is to not store the context in an xattr at all,
> and instead make a dedicated field in the inode for it that we can
> ensure is always present for encrypted inodes.  For the most part the
> crypto context is a static thing. The only exception is when we're first
> encrypting an empty dir.
>
> We already have the fscrypt bool in the inodestat, and we're going to
> need another field to hold the real size for files. It may be worthwhile
> to just reconsider the design at that level. Maybe we just need to carve
> out a chunk of fscrypt space in the inode for the client and let it
> manage that however it sees fit.

That's another solution.  Since the initial (naïfe) idea of having a
client-only implementation with fscrypt-agnostic MDSs is long gone, the
design can (still) be fixed to do that.  This will definitely allow to
move forward with the fscrypt implementation.  (But we'll probably be
bitten again with these recursive RPCs in the future!)

Anyway, this is probably the most interesting solution as it also reduces
the need for extra calls to MDS.  And the fscrypt bool in inodestat
probably becomes redundant and can be dropped.

Cheers,
-- 
Luis