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. -- Jeff Layton <jlayton@xxxxxxxxxx>
