Linux-Fsdevel Archive on lore.kernel.org
 help / color / Atom feed
From: Daniel Rosenberg <drosen@google.com>
To: Eric Biggers <ebiggers@kernel.org>,
	Al Viro <viro@zeniv.linux.org.uk>,
	Gabriel Krisman Bertazi <krisman@collabora.com>
Cc: "Theodore Ts'o" <tytso@mit.edu>,
	linux-ext4@vger.kernel.org, Jaegeuk Kim <jaegeuk@kernel.org>,
	Chao Yu <chao@kernel.org>,
	linux-f2fs-devel@lists.sourceforge.net,
	linux-fscrypt@vger.kernel.org,
	Richard Weinberger <richard@nod.at>,
	linux-mtd@lists.infradead.org,
	Andreas Dilger <adilger.kernel@dilger.ca>,
	Jonathan Corbet <corbet@lwn.net>,
	linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-fsdevel@vger.kernel.org, kernel-team@android.com
Subject: Re: [PATCH v7 2/8] fs: Add standard casefolding support
Date: Wed, 19 Feb 2020 18:27:17 -0800
Message-ID: <CA+PiJmRX1tBVqdAgHwk62rGqEQg28B3j5mEsaDBm3UV9_fzDEQ@mail.gmail.com> (raw)
In-Reply-To: <20200212065734.GA157327@sol.localdomain>

On Tue, Feb 11, 2020 at 10:57 PM Eric Biggers <ebiggers@kernel.org> wrote:
>
> Or (just throwing another idea out there) the dentry's name could be copied to a
> temporary buffer in ->d_compare().  The simplest version would be:
>
>         u8 _name[NAME_MAX];
>
>         memcpy(_name, name, len);
>         name = _name;
>
> Though, 255 bytes is a bit large for a stack buffer (so for long names it may
> need kmalloc with GFP_ATOMIC), and technically it would need a special version
> of memcpy() to be guaranteed safe from compiler optimizations (though I expect
> this would work in practice).
>
> Alternatively, take_dentry_name_snapshot() kind of does this already, except
> that it takes a dentry and not a (name, len) pair.
>
> - Eric

If we want to use take_dentry_name_snapshot, we'd need to do it before
calling the dentry op, since we get the dentry as a const. It would do
exactly what we want, in that it either takes a reference on the long
name, or copies the short name, although it does so under a spinlock.
I'm guessing we don't want to add that overhead for all
d_compare/d_hash's. I suppose it could just take a snapshot if it
falls under needs_casefold, but that feels a bit silly to me.

i don't think utf8cursor/utf8byte could be modified to be RCU safe
apart from a copy. As part of normalization there's some sorting that
goes on to ensure that different encodings of the same characters can
be matched, and I think those can technically be arbitrarily long, so
we'd possibly end up needing the copy anyways.

So, I see two possible fixes.
1. Use take_dentry_name_snapshot along the RCU paths to calling d_hash
and d_compare, at least when needs_casefold is true.
2. Within d_hash/d_compare, create a copy of the name if it is a short name.

For 1, it adds some overhead in general, which I'm sure we'd want to avoid.
For 2, I don't think we know we're in RCU mode, so we'd need to always
copy short filenames. I'm also unsure if it's valid to assume that
name given is stable if it is not the same as dentry->d_iname. If it
is, we only need to worry about copying DNAME_INLINE_LEN bytes at max
there. For memcpy, is there a different version that we'd want to use
for option 2?

-Daniel

  reply index

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-08  1:35 [PATCH v7 0/8] Support fof Casefolding and Encryption Daniel Rosenberg
2020-02-08  1:35 ` [PATCH v7 1/8] unicode: Add utf8_casefold_iter Daniel Rosenberg
2020-02-12  3:38   ` Eric Biggers
2020-02-14 21:47     ` Daniel Rosenberg
2020-02-17 19:02       ` Gabriel Krisman Bertazi
2020-02-08  1:35 ` [PATCH v7 2/8] fs: Add standard casefolding support Daniel Rosenberg
2020-02-08  2:12   ` Al Viro
2020-02-10 23:11     ` Daniel Rosenberg
2020-02-10 23:42       ` Al Viro
2020-02-12  6:34         ` Eric Biggers
2020-02-12  6:57           ` Eric Biggers
2020-02-20  2:27             ` Daniel Rosenberg [this message]
2020-02-12  3:55   ` Eric Biggers
2020-02-08  1:35 ` [PATCH v7 3/8] f2fs: Use generic " Daniel Rosenberg
2020-02-12  4:05   ` Eric Biggers
2020-02-08  1:35 ` [PATCH v7 4/8] ext4: " Daniel Rosenberg
2020-02-08  1:35 ` [PATCH v7 5/8] fscrypt: Have filesystems handle their d_ops Daniel Rosenberg
2020-02-12  4:33   ` Eric Biggers
2020-02-08  1:35 ` [PATCH v7 6/8] f2fs: Handle casefolding with Encryption Daniel Rosenberg
2020-02-12  5:10   ` Eric Biggers
2020-02-12  5:55     ` Al Viro
2020-02-12  6:06       ` Eric Biggers
2020-02-12  5:47   ` Eric Biggers
2020-02-08  1:35 ` [PATCH v7 7/8] ext4: Hande casefolding with encryption Daniel Rosenberg
2020-02-12  5:59   ` Eric Biggers
2020-02-08  1:35 ` [PATCH v7 8/8] ext4: Optimize match for casefolded encrypted dirs Daniel Rosenberg
2020-02-12  6:12 ` [PATCH v7 0/8] Support fof Casefolding and Encryption Eric Biggers
2020-02-13  0:01   ` Daniel Rosenberg

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=CA+PiJmRX1tBVqdAgHwk62rGqEQg28B3j5mEsaDBm3UV9_fzDEQ@mail.gmail.com \
    --to=drosen@google.com \
    --cc=adilger.kernel@dilger.ca \
    --cc=chao@kernel.org \
    --cc=corbet@lwn.net \
    --cc=ebiggers@kernel.org \
    --cc=jaegeuk@kernel.org \
    --cc=kernel-team@android.com \
    --cc=krisman@collabora.com \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-ext4@vger.kernel.org \
    --cc=linux-f2fs-devel@lists.sourceforge.net \
    --cc=linux-fscrypt@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mtd@lists.infradead.org \
    --cc=richard@nod.at \
    --cc=tytso@mit.edu \
    --cc=viro@zeniv.linux.org.uk \
    /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

Linux-Fsdevel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-fsdevel/0 linux-fsdevel/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 linux-fsdevel linux-fsdevel/ https://lore.kernel.org/linux-fsdevel \
		linux-fsdevel@vger.kernel.org
	public-inbox-index linux-fsdevel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-fsdevel


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