CEPH-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Jeff Layton <jlayton@kernel.org>
To: Luis Henriques <lhenriques@suse.de>
Cc: ceph-devel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	linux-fscrypt@vger.kernel.org
Subject: Re: [RFC PATCH v6 00/20] ceph+fscrypt: context, filename and symlink support
Date: Tue, 20 Apr 2021 11:52:07 -0400
Message-ID: <53d5bebb28c1e0cd354a336a56bf103d5e3a6344.camel@kernel.org> (raw)
In-Reply-To: <87sg3ll0zm.fsf@suse.de>

On Tue, 2021-04-20 at 11:11 +0100, Luis Henriques wrote:
> 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.
> 

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@kernel.org>


      reply index

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-13 17:50 Jeff Layton
2021-04-13 17:50 ` [RFC PATCH v6 01/20] vfs: export new_inode_pseudo Jeff Layton
2021-04-13 17:50 ` [RFC PATCH v6 02/20] fscrypt: export fscrypt_base64_encode and fscrypt_base64_decode Jeff Layton
2021-04-13 17:50 ` [RFC PATCH v6 03/20] fscrypt: export fscrypt_fname_encrypt and fscrypt_fname_encrypted_size Jeff Layton
2021-04-13 17:50 ` [RFC PATCH v6 04/20] fscrypt: add fscrypt_context_for_new_inode Jeff Layton
2021-04-13 17:50 ` [RFC PATCH v6 05/20] ceph: crypto context handling for ceph Jeff Layton
2021-04-13 17:50 ` [RFC PATCH v6 06/20] ceph: implement -o test_dummy_encryption mount option Jeff Layton
2021-04-13 17:50 ` [RFC PATCH v6 07/20] ceph: preallocate inode for ops that may create one Jeff Layton
2021-04-13 17:50 ` [RFC PATCH v6 08/20] ceph: add routine to create fscrypt context prior to RPC Jeff Layton
2021-04-13 17:50 ` [RFC PATCH v6 09/20] ceph: make ceph_msdc_build_path use ref-walk Jeff Layton
2021-04-13 17:50 ` [RFC PATCH v6 10/20] ceph: add encrypted fname handling to ceph_mdsc_build_path Jeff Layton
2021-04-13 17:50 ` [RFC PATCH v6 11/20] ceph: decode alternate_name in lease info Jeff Layton
2021-04-13 17:50 ` [RFC PATCH v6 12/20] ceph: send altname in MClientRequest Jeff Layton
2021-04-13 17:50 ` [RFC PATCH v6 13/20] ceph: properly set DCACHE_NOKEY_NAME flag in lookup Jeff Layton
2021-04-13 17:50 ` [RFC PATCH v6 14/20] ceph: make d_revalidate call fscrypt revalidator for encrypted dentries Jeff Layton
2021-04-13 17:50 ` [RFC PATCH v6 15/20] ceph: add helpers for converting names for userland presentation Jeff Layton
2021-04-13 17:50 ` [RFC PATCH v6 16/20] ceph: add fscrypt support to ceph_fill_trace Jeff Layton
2021-04-13 17:50 ` [RFC PATCH v6 17/20] ceph: add support to readdir for encrypted filenames Jeff Layton
2021-04-13 17:50 ` [RFC PATCH v6 18/20] ceph: create symlinks with encrypted and base64-encoded targets Jeff Layton
2021-04-13 17:50 ` [RFC PATCH v6 19/20] ceph: make ceph_get_name decrypt filenames Jeff Layton
2021-04-13 17:50 ` [RFC PATCH v6 20/20] ceph: add fscrypt ioctls Jeff Layton
2021-04-19 10:09   ` Luis Henriques
2021-04-19 12:19     ` Jeff Layton
2021-04-19 19:54       ` Eric Biggers
2021-04-20  9:34         ` Luis Henriques
2021-04-20 11:45         ` Jeff Layton
2021-04-19 10:30 ` [RFC PATCH v6 00/20] ceph+fscrypt: context, filename and symlink support Luis Henriques
2021-04-19 12:23   ` Jeff Layton
2021-04-19 16:03     ` Luis Henriques
2021-04-19 16:28       ` Jeff Layton
2021-04-20 10:11         ` Luis Henriques
2021-04-20 15:52           ` Jeff Layton [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=53d5bebb28c1e0cd354a336a56bf103d5e3a6344.camel@kernel.org \
    --to=jlayton@kernel.org \
    --cc=ceph-devel@vger.kernel.org \
    --cc=lhenriques@suse.de \
    --cc=linux-fscrypt@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

CEPH-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/ceph-devel/0 ceph-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 ceph-devel ceph-devel/ https://lore.kernel.org/ceph-devel \
		ceph-devel@vger.kernel.org
	public-inbox-index ceph-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.ceph-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git