On Wed, Aug 13, 2025 at 12:57:49AM +0100, Tingmao Wang wrote:
> Thanks for the review :) I will try to send a v2 in the coming weeks with
> the two changes you suggested and the changes to cached mode as suggested
> by Dominique (plus rename handling). (will also try to figure out how to
> test with xfstests)
>
> On 8/8/25 09:32, Mickaël Salaün wrote:
> > [...]
> >> On 7/5/25 01:25, Al Viro wrote:
> >>> On Sun, Apr 06, 2025 at 09:43:02PM +0100, Tingmao Wang wrote:
> >>>> +bool ino_path_compare(struct v9fs_ino_path *ino_path,
> >>>> + struct dentry *dentry)
> >>>> +{
> >>>> + struct dentry *curr = dentry;
> >>>> + struct qstr *curr_name;
> >>>> + struct name_snapshot *compare;
> >>>> + ssize_t i;
> >>>> +
> >>>> + lockdep_assert_held_read(&v9fs_dentry2v9ses(dentry)->rename_sem);
> >>>> +
> >>>> + rcu_read_lock();
> >>>> + for (i = ino_path->nr_components - 1; i >= 0; i--) {
> >>>> + if (curr->d_parent == curr) {
> >>>> + /* We're supposed to have more components to walk */
> >>>> + rcu_read_unlock();
> >>>> + return false;
> >>>> + }
> >>>> + curr_name = &curr->d_name;
> >>>> + compare = &ino_path->names[i];
> >>>> + /*
> >>>> + * We can't use hash_len because it is salted with the parent
> >>>> + * dentry pointer. We could make this faster by pre-computing our
> >>>> + * own hashlen for compare and ino_path outside, probably.
> >>>> + */
> >>>> + if (curr_name->len != compare->name.len) {
> >>>> + rcu_read_unlock();
> >>>> + return false;
> >>>> + }
> >>>> + if (strncmp(curr_name->name, compare->name.name,
> >>>> + curr_name->len) != 0) {
> >>>
> >>> ... without any kind of protection for curr_name. Incidentally,
> >>> what about rename()? Not a cross-directory one, just one that
> >>> changes the name of a subdirectory within the same parent?
> >>
> >> As far as I can tell, in v9fs_vfs_rename, v9ses->rename_sem is taken for
> >> both same-parent and different parent renames, so I think we're safe here
> >> (and hopefully for any v9fs dentries, nobody should be causing d_name to
> >> change except for ourselves when we call d_move in v9fs_vfs_rename? If
> >> yes then because we also take v9ses->rename_sem, in theory we should be
> >> fine here...?)
> >
> > A lockdep_assert_held() or similar and a comment would make this clear.
>
> I can add a comment, but there is already a lockdep_assert_held_read of
> the v9fs rename sem at the top of this function.
I wrote this comment before reading your new version beneath, which
already have this lockdep, so no need to change anything. :)
>
> > [...]
> >> /*
> >> * Must hold rename_sem due to traversing parents
> >> */
> >> bool ino_path_compare(struct v9fs_ino_path *ino_path, struct dentry *dentry)
> >> {
> >> struct dentry *curr = dentry;
> >> struct name_snapshot *compare;
> >> ssize_t i;
> >>
> >> lockdep_assert_held_read(&v9fs_dentry2v9ses(dentry)->rename_sem);
> >>
> >> rcu_read_lock();
> >> for (i = ino_path->nr_components - 1; i >= 0; i--) {
> >> if (curr->d_parent == curr) {
> >> /* We're supposed to have more components to walk */
> >> rcu_read_unlock();
> >> return false;
> >> }
> >> compare = &ino_path->names[i];
> >> if (!d_same_name(curr, curr->d_parent, &compare->name)) {
> >> rcu_read_unlock();
> >> return false;
> >> }
> >> curr = curr->d_parent;
> >> }
> >> rcu_read_unlock();
> >> if (curr != curr->d_parent) {
>
> Looking at this again I think this check probably needs to be done inside
> RCU, will fix as below:
>
> >> /* dentry is deeper than ino_path */
> >> return false;
> >> }
> >> return true;
> >> }
>
> diff --git a/fs/9p/ino_path.c b/fs/9p/ino_path.c
> index 0000b4964df0..7264003cb087 100644
> --- a/fs/9p/ino_path.c
> +++ b/fs/9p/ino_path.c
> @@ -77,13 +77,15 @@ void free_ino_path(struct v9fs_ino_path *path)
> }
>
> /*
> - * Must hold rename_sem due to traversing parents
> + * Must hold rename_sem due to traversing parents. Returns whether
> + * ino_path matches with the path of a v9fs dentry.
> */
> bool ino_path_compare(struct v9fs_ino_path *ino_path, struct dentry *dentry)
> {
> struct dentry *curr = dentry;
> struct name_snapshot *compare;
> ssize_t i;
> + bool ret;
>
> lockdep_assert_held_read(&v9fs_dentry2v9ses(dentry)->rename_sem);
>
> @@ -101,10 +103,8 @@ bool ino_path_compare(struct v9fs_ino_path *ino_path, struct dentry *dentry)
> }
> curr = curr->d_parent;
> }
> + /* Comparison fails if dentry is deeper than ino_path */
> + ret = (curr == curr->d_parent);
> rcu_read_unlock();
> - if (curr != curr->d_parent) {
> - /* dentry is deeper than ino_path */
> - return false;
> - }
> - return true;
> + return ret;
> }
Looks good
>
> >
> > I like this new version.
> >
>
