From: Al Viro <viro@zeniv.linux.org.uk>
To: Gabriel Krisman Bertazi <gabriel@krisman.be>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
Christian Brauner <brauner@kernel.org>,
tytso@mit.edu, linux-f2fs-devel@lists.sourceforge.net,
ebiggers@kernel.org, linux-fsdevel@vger.kernel.org,
jaegeuk@kernel.org, linux-ext4@vger.kernel.org
Subject: Re: [f2fs-dev] [PATCH v6 0/9] Support negative dentries on case-insensitive ext4 and f2fs
Date: Thu, 23 Nov 2023 17:12:55 +0000 [thread overview]
Message-ID: <20231123171255.GN38156@ZenIV> (raw)
In-Reply-To: <87o7fkihst.fsf@>
On Thu, Nov 23, 2023 at 10:57:22AM -0500, Gabriel Krisman Bertazi wrote:
> Linus Torvalds <torvalds@linux-foundation.org> writes:
>
> > Side note: Gabriel, as things are now, instead of that
> >
> > if (!d_is_casefolded_name(dentry))
> > return 0;
> >
> > in generic_ci_d_revalidate(), I would suggest that any time a
> > directory is turned into a case-folded one, you'd just walk all the
> > dentries for that directory and invalidate negative ones at that
> > point. Or was there some reason I missed that made it a good idea to
> > do it at run-time after-the-fact?
> >
>
> The problem I found with that approach, which I originally tried, was
> preventing concurrent lookups from racing with the invalidation and
> creating more 'case-sensitive' negative dentries. Did I miss a way to
> synchronize with concurrent lookups of the children of the dentry? We
> can trivially ensure the dentry doesn't have positive children by
> holding the parent lock, but that doesn't protect from concurrent
> lookups creating negative dentries, as far as I understand.
AFAICS, there is a problem with dentries that never came through
->lookup(). Unless I'm completely misreading your code, your
generic_ci_d_revalidate() is not called for them. Ever.
Hash lookups are controlled by ->d_op of parent; that's where ->d_hash()
and ->d_compare() come from. Revalidate comes from *child*. You need
->d_op->d_revalidate of child dentry to be set to your generic_ci_d_revalidate().
The place where it gets set is generic_set_encrypted_ci_d_ops(). Look
at its callchain; in case of ext4 it gets called from ext4_lookup_dentry(),
which is called from ext4_lookup(). And dentry passed to it is the
argument of ->lookup().
Now take a look at open-by-fhandle stuff; all methods in there
(->fh_to_dentry(), ->fh_to_parent(), ->get_parent()) end up
returning d_obtain_alias(some inode).
We *do* call ->lookup(), all right - in reconnect_one(), while
trying to connect those suckers with the main tree. But the way
it works is that d_splice_alias() in ext4_lookup() moves the
existing alias for subdirectory, connecting it to the parent.
That's not the dentry ext4_lookup() had set ->d_op on - that's
the dentry that came from d_obtain_alias(). And those do not
have ->d_op set by anything in your tree.
That's the problem I'd been talking about - there is a class of situations
where the work done by ext4_lookup() to set the state of dentry gets
completely lost. After lookup you do have a dentry in the right place,
with the right name and inode, etc., but with NULL ->d_op->d_revalidate.
next prev parent reply other threads:[~2023-11-23 17:13 UTC|newest]
Thread overview: 65+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-08-16 5:07 [PATCH v6 0/9] Support negative dentries on case-insensitive ext4 and f2fs Gabriel Krisman Bertazi
2023-08-16 5:07 ` [PATCH v6 1/9] ecryptfs: Reject casefold directory inodes Gabriel Krisman Bertazi
2023-08-16 5:07 ` [PATCH v6 2/9] 9p: Split ->weak_revalidate from ->revalidate Gabriel Krisman Bertazi
2023-08-16 5:07 ` [PATCH v6 3/9] fs: Expose name under lookup to d_revalidate hooks Gabriel Krisman Bertazi
2023-11-22 20:59 ` Al Viro
2023-08-16 5:07 ` [PATCH v6 4/9] fs: Add DCACHE_CASEFOLDED_NAME flag Gabriel Krisman Bertazi
2023-11-22 20:32 ` Al Viro
2023-08-16 5:07 ` [PATCH v6 5/9] libfs: Validate negative dentries in case-insensitive directories Gabriel Krisman Bertazi
2023-11-22 20:20 ` Al Viro
2023-08-16 5:08 ` [PATCH v6 6/9] libfs: Chain encryption checks after case-insensitive revalidation Gabriel Krisman Bertazi
2023-08-16 5:08 ` [PATCH v6 7/9] libfs: Merge encrypted_ci_dentry_ops and ci_dentry_ops Gabriel Krisman Bertazi
2023-08-16 5:08 ` [PATCH v6 8/9] ext4: Enable negative dentries on case-insensitive lookup Gabriel Krisman Bertazi
2023-08-16 5:08 ` [PATCH v6 9/9] f2fs: " Gabriel Krisman Bertazi
2023-08-17 17:06 ` [PATCH v6 0/9] Support negative dentries on case-insensitive ext4 and f2fs Eric Biggers
2023-08-21 15:52 ` Christian Brauner
2023-08-21 18:53 ` Gabriel Krisman Bertazi
2023-08-22 9:03 ` Christian Brauner
2023-10-24 22:20 ` Gabriel Krisman Bertazi
2023-10-25 13:32 ` Christian Brauner
2023-10-25 15:19 ` Gabriel Krisman Bertazi
2023-11-19 23:11 ` [f2fs-dev] " Gabriel Krisman Bertazi
[not found] ` <655a9634.630a0220.d50d7.5063SMTPIN_ADDED_BROKEN@mx.google.com>
2023-11-20 15:06 ` Christian Brauner
2023-11-20 16:59 ` Gabriel Krisman Bertazi
2023-11-20 18:07 ` Linus Torvalds
2023-11-21 2:02 ` Theodore Ts'o
2023-11-21 2:29 ` Linus Torvalds
2023-11-21 3:03 ` Linus Torvalds
2023-11-21 5:12 ` Theodore Ts'o
2023-11-22 21:04 ` Al Viro
2023-11-21 2:27 ` Al Viro
2023-11-22 21:19 ` Al Viro
2023-11-23 0:18 ` Linus Torvalds
2023-11-23 5:09 ` Al Viro
2023-11-23 15:57 ` Gabriel Krisman Bertazi
2023-11-23 17:12 ` Al Viro [this message]
2023-11-23 17:37 ` Gabriel Krisman Bertazi
2023-11-23 18:24 ` Al Viro
2023-11-23 19:06 ` Gabriel Krisman Bertazi
2023-11-23 19:53 ` Al Viro
2023-11-23 20:15 ` Al Viro
2023-11-24 15:20 ` Gabriel Krisman Bertazi
2023-11-28 0:02 ` Al Viro
2023-11-23 21:52 ` Al Viro
2023-11-24 15:22 ` Gabriel Krisman Bertazi
2023-11-25 22:01 ` Al Viro
2023-11-26 4:52 ` Al Viro
2023-11-26 18:41 ` fun with d_invalidate() vs. d_splice_alias() was " Al Viro
2023-11-27 6:38 ` Al Viro
2023-11-27 15:47 ` Eric W. Biederman
2023-11-27 16:01 ` Eric W. Biederman
2023-11-27 17:25 ` Al Viro
2023-11-27 18:26 ` Al Viro
2023-11-27 16:03 ` Al Viro
2023-11-27 16:14 ` Al Viro
2023-11-27 18:19 ` Eric W. Biederman
2023-11-27 18:43 ` Al Viro
2023-11-27 16:33 ` Christian Brauner
2023-11-29 4:53 ` Al Viro
2023-11-29 10:21 ` Christian Brauner
2023-11-29 15:19 ` Eric W. Biederman
[not found] ` <655f7665.df0a0220.58a21.e84fSMTPIN_ADDED_BROKEN@mx.google.com>
2023-11-23 16:41 ` Linus Torvalds
2023-11-23 1:12 ` Al Viro
2023-11-23 1:22 ` Al Viro
2023-11-22 3:30 ` Gabriel Krisman Bertazi
2024-01-16 19:02 ` patchwork-bot+f2fs
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=20231123171255.GN38156@ZenIV \
--to=viro@zeniv.linux.org.uk \
--cc=brauner@kernel.org \
--cc=ebiggers@kernel.org \
--cc=gabriel@krisman.be \
--cc=jaegeuk@kernel.org \
--cc=linux-ext4@vger.kernel.org \
--cc=linux-f2fs-devel@lists.sourceforge.net \
--cc=linux-fsdevel@vger.kernel.org \
--cc=torvalds@linux-foundation.org \
--cc=tytso@mit.edu \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).