On Tue, 2021-04-20 at 11:11 +0100, Luis Henriques wrote: > Jeff Layton <jlayton@xxxxxxxxxx> writes: > > > On Mon, 2021-04-19 at 17:03 +0100, Luis Henriques wrote: > > > Jeff Layton <jlayton@xxxxxxxxxx> 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. > We probably can't drop the bool from the protocol, as it's now in a released version (Pacific). What we can do is drop tracking the bool internally in the MDS, and just set that to true if the fscrypt blob isn't zero-length. Cheers, -- Jeff Layton <jlayton@xxxxxxxxxx>
