CEPH-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Luis Henriques <lhenriques@suse.de>
To: Jeff Layton <jlayton@kernel.org>
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:11:09 +0100
Message-ID: <87sg3ll0zm.fsf@suse.de> (raw)
In-Reply-To: <13750c0b72dccd84e75179d62e9a9038d6f57371.camel@kernel.org> (Jeff Layton's message of "Mon, 19 Apr 2021 12:28:10 -0400")

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

  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 [this message]
2021-04-20 15:52           ` Jeff Layton

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=87sg3ll0zm.fsf@suse.de \
    --to=lhenriques@suse.de \
    --cc=ceph-devel@vger.kernel.org \
    --cc=jlayton@kernel.org \
    --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