Linux CIFS filesystem development
 help / color / mirror / Atom feed
* [PATCH v2 0/3] smb/client(cifs): prepare for VFS locking changes.
@ 2026-09-29  2:40 NeilBrown
  2026-09-29  2:40 ` [PATCH v2 1/3] smb/client: don't unhash and rehash to prevent new opens NeilBrown
                   ` (4 more replies)
  0 siblings, 5 replies; 8+ messages in thread
From: NeilBrown @ 2026-09-29  2:40 UTC (permalink / raw)
  To: Paulo Alcantara, Namjae Jeon, Alexander Viro, Christian Brauner
  Cc: Jeff Layton, Jan Kara, linux-fsdevel, linux-cifs, linux-kernel

The previous version of this series had a couple of problems reported by sashiko,
both in 2/3.
1/ an error from cifs_lookup() wasn't handled properly - it should be fatal
2/ The d_is_positive() tested the wrong dentry.

Original intro message below.
Thanks,
NeilBrown


I am working on changes to directory locking.  The medium term goal is
to lift d_alloc_parallel() locking above i_rwsem, so d_alloc_paralle()
can be run unlocked, and consequently cannot be called when ->i_rwsem is
held.

The requires:
 1/ that d_alloc_parallel() not be called while i_rwsem is held.  cifs
    does this in a couple of places.  A new interface d_alloc_trylock()
    is now available and can be used instead.

 2/ a dentry must not be d_drop()ed while an operation is ongoing, as an
    unlocked d_alloc_parallel() would then be able to create a new
    dentry with the same name.  In particular d_rehash() must be phased
    out as it won't be safe.  This particularly affects the mechanism to
    block lookups/revalidates while an unlink/rename is happening.  The
    mechanism is changed to use one similar to how NFS addresses this
    needed, though using the new LOOKUP_PRIVATE flag.

The patches depend on patches recently added to the vfs tree.  I would
prefer these land in that tree too with an Acked-by or similar from cifs
maintainers.

Thanks,
NeilBrown

 [PATCH v2 1/3] smb/client: don't unhash and rehash to prevent new
 [PATCH v2 2/3] smb/client: use d_alloc_trylock in
 [PATCH v2 3/3] smb/client: Use d_alloc_trylock() in

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2 1/3] smb/client: don't unhash and rehash to prevent new opens.
  2026-09-29  2:40 [PATCH v2 0/3] smb/client(cifs): prepare for VFS locking changes NeilBrown
@ 2026-09-29  2:40 ` NeilBrown
  2026-09-29  2:40 ` [PATCH v2 2/3] smb/client: use d_alloc_trylock in cifs_silly_fullpath() NeilBrown
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 8+ messages in thread
From: NeilBrown @ 2026-09-29  2:40 UTC (permalink / raw)
  To: Paulo Alcantara, Namjae Jeon, Alexander Viro, Christian Brauner
  Cc: Jeff Layton, Jan Kara, linux-fsdevel, linux-cifs, linux-kernel

From: NeilBrown <neil@brown.name>

smb/client needs to block new opens of the target of unlink and rename
while the operation is progressing.  This stablises d_count() and allows
a determination of whether a "silly-rename" is required.

It currently unhashes the dentry which will cause lookup to block on
the parent directory i_rwsem.  Proposed changes to locking will cause
this approach to stop working as the exclusivity will be provided for
the dentry only, and only while it is hashed.

So we introduce a new machanism similar to that used by nfs.
DCACHE_PRIVATE (given the name DCACHE_BLOCKED) is set when lookups need
to be blocked.  ->d_revalidate checks for this and blocks.  This might
still allow d_count() to increment, but once it has been tested as 1,
there can be no new opens completed.

Unlike unhash which does not need to be reverted on error, and which
must not be reverted on a successful d_move, blocking of opens must
always be reverted.  So we don't block the open until after the last
early "return", and we always unblock on the final "return".

Strictly speaking it is only necessary to block opens before the value
of d_count() is tested under ->d_lock. But we block as early as possible
to discourage new opens from starting.  As open is path-based in cifs a
concurrent rename can be confused a rename of one other the paths.  Once
cifs_open() has generated the full_path it doesn't hold any locks to
prevent a rename from making that path invalid.

The important details of the interlock between __cifs_unlink (which both
unlink and rename use) and "open" are that __cifs_unlink() sets
DCACHE_BLOCKED  *before* testing d_count() which is
in a d_lock locked region,  and cifs_d_revalidate() tests the bit *after*
->d_lock is taken by e.g. __d_lookup() to increment d_count().
Thus ->d_lock provide serialization between the set and the test
even though the test isn't in a locked region.
(The fact that cifs_d_revalidate() always returned -ECHILD when
 LOOKUP_RCU is important for this sequencing to work as it ensures
 d_revalidate() is called *after* d_count() is incremented).

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/smb/client/cifsfs.h |  8 +++++
 fs/smb/client/dir.c    |  3 ++
 fs/smb/client/inode.c  | 68 ++++++++++++++++++++++++------------------
 3 files changed, 50 insertions(+), 29 deletions(-)

diff --git a/fs/smb/client/cifsfs.h b/fs/smb/client/cifsfs.h
index 0c85daa8386e..bcc0fb5c2c9b 100644
--- a/fs/smb/client/cifsfs.h
+++ b/fs/smb/client/cifsfs.h
@@ -42,6 +42,14 @@ static inline unsigned long cifs_get_time(struct dentry *dentry)
 	return (unsigned long) dentry->d_fsdata;
 }
 
+/*
+ * This is set to block d_revalidate on a dentry that is being removed -
+ * the target of unlink or rename.  This causes any open attempt to
+ * block.  There may be existing opens but they can be detected by
+ * checking d_count() under ->d_lock.
+ */
+#define DCACHE_BLOCKED DCACHE_PRIVATE
+
 extern struct file_system_type cifs_fs_type, smb3_fs_type;
 extern const struct address_space_operations cifs_addr_ops;
 extern const struct address_space_operations cifs_addr_ops_smallbuf;
diff --git a/fs/smb/client/dir.c b/fs/smb/client/dir.c
index 6fa6d48fdfd3..88fff65320f5 100644
--- a/fs/smb/client/dir.c
+++ b/fs/smb/client/dir.c
@@ -872,6 +872,9 @@ cifs_d_revalidate(struct inode *dir, const struct qstr *name,
 	if (flags & LOOKUP_RCU)
 		return -ECHILD;
 
+	/* Wait for pending rename/unlink */
+	wait_var_event(&direntry->d_flags, !(direntry->d_flags & DCACHE_BLOCKED));
+
 	if (d_really_is_positive(direntry)) {
 		int rc;
 		struct inode *inode = d_inode(direntry);
diff --git a/fs/smb/client/inode.c b/fs/smb/client/inode.c
index 12ed8db10e00..4d34c5622cd4 100644
--- a/fs/smb/client/inode.c
+++ b/fs/smb/client/inode.c
@@ -1967,24 +1967,31 @@ static int __cifs_unlink(struct inode *dir, struct dentry *dentry, bool sillyren
 	__u32 dosattr = 0, origattr = 0;
 	struct TCP_Server_Info *server;
 	struct iattr *attrs = NULL;
-	bool rehash = false;
+	bool unblock = false;
 
 	cifs_dbg(FYI, "cifs_unlink, dir=0x%p, dentry=0x%p\n", dir, dentry);
 
 	if (unlikely(cifs_forced_shutdown(cifs_sb)))
 		return smb_EIO(smb_eio_trace_forced_shutdown);
 
-	/* Unhash dentry in advance to prevent any concurrent opens */
-	spin_lock(&dentry->d_lock);
-	if (!d_unhashed(dentry)) {
-		__d_drop(dentry);
-		rehash = true;
-	}
-	spin_unlock(&dentry->d_lock);
-
 	tlink = cifs_sb_tlink(cifs_sb);
 	if (IS_ERR(tlink))
 		return PTR_ERR(tlink);
+
+	/* opens might already be blocked by rename */
+	if (!(dentry->d_flags & DCACHE_BLOCKED)) {
+		/*
+		 * Block opens - and all lookups that involve d_revalidate.
+		 * This guarantees that if another thread tries to open(), it
+		 * will either block, or will increment d_count()
+		 * before we test it below.
+		 */
+		spin_lock(&dentry->d_lock);
+		dentry->d_flags |= DCACHE_BLOCKED;
+		spin_unlock(&dentry->d_lock);
+		unblock = true;
+	}
+
 	tcon = tlink_tcon(tlink);
 	server = tcon->ses->server;
 
@@ -2107,8 +2114,13 @@ static int __cifs_unlink(struct inode *dir, struct dentry *dentry, bool sillyren
 	kfree(attrs);
 	free_xid(xid);
 	cifs_put_tlink(tlink);
-	if (rehash)
-		d_rehash(dentry);
+	/* Allow lookups/opens */
+	if (unblock) {
+		spin_lock(&dentry->d_lock);
+		store_release_wake_up(&dentry->d_flags,
+				      dentry->d_flags &~ DCACHE_BLOCKED);
+		spin_unlock(&dentry->d_lock);
+	}
 	return rc;
 }
 
@@ -2536,7 +2548,6 @@ cifs_rename2(struct mnt_idmap *idmap, struct inode *source_dir,
 	struct cifs_sb_info *cifs_sb;
 	struct tcon_link *tlink;
 	struct cifs_tcon *tcon;
-	bool rehash = false;
 	unsigned int xid;
 	int rc, tmprc;
 	int retry_count = 0;
@@ -2552,20 +2563,20 @@ cifs_rename2(struct mnt_idmap *idmap, struct inode *source_dir,
 	if (unlikely(cifs_forced_shutdown(cifs_sb)))
 		return smb_EIO(smb_eio_trace_forced_shutdown);
 
-	/*
-	 * Prevent any concurrent opens on the target by unhashing the dentry.
-	 * VFS already unhashes the target when renaming directories.
-	 */
-	if (d_is_positive(target_dentry) && !d_is_dir(target_dentry)) {
-		if (!d_unhashed(target_dentry)) {
-			d_drop(target_dentry);
-			rehash = true;
-		}
-	}
-
 	tlink = cifs_sb_tlink(cifs_sb);
 	if (IS_ERR(tlink))
 		return PTR_ERR(tlink);
+
+	/*
+	 * Block opens - and all lookups that involve d_revalidate.
+	 * This guarantees that if another thread tries to open(), it
+	 * will either block, or will increment d_count()
+	 * before we test it in __cifs_unlink().
+	 */
+	spin_lock(&target_dentry->d_lock);
+	target_dentry->d_flags |= DCACHE_BLOCKED;
+	spin_unlock(&target_dentry->d_lock);
+
 	tcon = tlink_tcon(tlink);
 	server = tcon->ses->server;
 
@@ -2605,8 +2616,6 @@ cifs_rename2(struct mnt_idmap *idmap, struct inode *source_dir,
 		}
 	}
 
-	if (!rc)
-		rehash = false;
 	/*
 	 * No-replace is the natural behavior for CIFS, so skip unlink hacks.
 	 */
@@ -2698,8 +2707,6 @@ cifs_rename2(struct mnt_idmap *idmap, struct inode *source_dir,
 			}
 			rc = cifs_do_rename(xid, source_dentry, from_name,
 					    target_dentry, to_name);
-			if (!rc)
-				rehash = false;
 		}
 	}
 
@@ -2713,8 +2720,11 @@ cifs_rename2(struct mnt_idmap *idmap, struct inode *source_dir,
 	CIFS_I(source_dir)->time = CIFS_I(target_dir)->time = 0;
 
 cifs_rename_exit:
-	if (rehash)
-		d_rehash(target_dentry);
+	/* Allow lookups/opens */
+	spin_lock(&target_dentry->d_lock);
+	store_release_wake_up(&target_dentry->d_flags,
+			      target_dentry->d_flags &~ DCACHE_BLOCKED);
+	spin_unlock(&target_dentry->d_lock);
 	kfree(info_buf_source);
 	free_dentry_path(page2);
 	free_dentry_path(page1);

base-commit: 3879f51857325da9bf3cfb073280257cd16ae067
-- 
2.50.0.107.gf914562f5916.dirty


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 2/3] smb/client: use d_alloc_trylock in cifs_silly_fullpath()
  2026-09-29  2:40 [PATCH v2 0/3] smb/client(cifs): prepare for VFS locking changes NeilBrown
  2026-09-29  2:40 ` [PATCH v2 1/3] smb/client: don't unhash and rehash to prevent new opens NeilBrown
@ 2026-09-29  2:40 ` NeilBrown
  2026-09-29  2:40 ` [PATCH v2 3/3] smb/client: Use d_alloc_trylock() in cifs_prime_dcache() NeilBrown
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 8+ messages in thread
From: NeilBrown @ 2026-09-29  2:40 UTC (permalink / raw)
  To: Paulo Alcantara, Namjae Jeon, Alexander Viro, Christian Brauner
  Cc: Jeff Layton, Jan Kara, linux-fsdevel, linux-cifs, linux-kernel

From: NeilBrown <neil@brown.name>

Rather than performing a normal lookup (which will be awkward with future
locking changes) use d_alloc_trylock() to find a dentry for an
unused name, and use an open-coded lookup_slow() to see if it is free on
the server.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/smb/client/dir.c | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/fs/smb/client/dir.c b/fs/smb/client/dir.c
index 88fff65320f5..70ca3da28913 100644
--- a/fs/smb/client/dir.c
+++ b/fs/smb/client/dir.c
@@ -1183,23 +1183,36 @@ char *cifs_silly_fullpath(struct dentry *dentry)
 	unsigned char name[CIFS_SILLYNAME_LEN + 1];
 	int retries = 0, max_retries = 16;
 	size_t namesize = sizeof(name);
-	struct dentry *sdentry = NULL;
+	struct dentry *sdentry = NULL, *alias;
 	char *path;
 
 	do {
 		dput(sdentry);
 		scnprintf(name, namesize, CIFS_SILLYNAME_PREFIX "%x",
 			  atomic_inc_return(&cifs_sillycounter));
-		sdentry = lookup_noperm(&QSTR(name), dentry->d_parent);
+		sdentry = d_alloc_trylock(dentry->d_parent, &QSTR(name));
+		if (sdentry == ERR_PTR(-EWOULDBLOCK)) {
+			sdentry = NULL;
+			continue;
+		}
 		if (IS_ERR(sdentry))
 			return ERR_CAST(sdentry);
-		if (d_is_negative(sdentry)) {
-			dput(sdentry);
-			path = alloc_parent_path(dentry, CIFS_SILLYNAME_LEN);
-			if (!IS_ERR(path))
-				strcat(path, name);
-			return path;
+		if (!d_in_lookup(sdentry))
+			continue;
+		alias = cifs_lookup(sdentry->d_parent->d_inode, sdentry, 0);
+		d_lookup_done(sdentry);
+		/* It is safest to return -EBUSY if there is an error in lookup */
+		if (IS_ERR(alias))
+			break;
+		if (alias || d_is_positive(sdentry)) {
+			dput(alias);
+			continue;
 		}
+		dput(sdentry);
+		path = alloc_parent_path(dentry, CIFS_SILLYNAME_LEN);
+		if (!IS_ERR(path))
+			strcat(path, name);
+		return path;
 	} while (++retries < max_retries);
 	dput(sdentry);
 	return ERR_PTR(-EBUSY);
-- 
2.50.0.107.gf914562f5916.dirty


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 3/3] smb/client: Use d_alloc_trylock() in cifs_prime_dcache()
  2026-09-29  2:40 [PATCH v2 0/3] smb/client(cifs): prepare for VFS locking changes NeilBrown
  2026-09-29  2:40 ` [PATCH v2 1/3] smb/client: don't unhash and rehash to prevent new opens NeilBrown
  2026-09-29  2:40 ` [PATCH v2 2/3] smb/client: use d_alloc_trylock in cifs_silly_fullpath() NeilBrown
@ 2026-09-29  2:40 ` NeilBrown
  2026-10-23  0:02   ` Paulo Alcantara
  2026-10-21 13:43 ` [PATCH v2 0/3] smb/client(cifs): prepare for VFS locking changes Namjae Jeon
  2026-10-23  0:03 ` Paulo Alcantara
  4 siblings, 1 reply; 8+ messages in thread
From: NeilBrown @ 2026-09-29  2:40 UTC (permalink / raw)
  To: Paulo Alcantara, Namjae Jeon, Alexander Viro, Christian Brauner
  Cc: Jeff Layton, Jan Kara, linux-fsdevel, linux-cifs, linux-kernel

From: NeilBrown <neil@brown.name>

cifs uses the results of readdir to prime the dcache.  Using
d_alloc_parallel() can block if there is a concurrent lookup.  Blocking
in that case is pointless as the lookup will add info to the dcache and
there is no value in the readdir waiting to see if it should add the
info too.

Also this call to d_alloc_parallel() is made while the parent
directory is locked.  A proposed change to locking will lock the parent
later, after d_alloc_parallel().  This means it won't be safe to wait in
d_alloc_parallel() while holding the directory lock.

So change to use d_alloc_trylock().  As d_alloc_trylock() includes a
try_lookup we don't need both.   Rearrange the code so we can choose
either a try_lookup or d_alloc_trylock.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/smb/client/readdir.c | 50 ++++++++++++++++++++---------------------
 1 file changed, 24 insertions(+), 26 deletions(-)

diff --git a/fs/smb/client/readdir.c b/fs/smb/client/readdir.c
index 32a75afca8f5..19d0c77046ab 100644
--- a/fs/smb/client/readdir.c
+++ b/fs/smb/client/readdir.c
@@ -77,36 +77,34 @@ cifs_prime_dcache(struct dentry *parent, struct qstr *name,
 
 	cifs_dbg(FYI, "%s: for %s\n", __func__, name->name);
 
-	dentry = try_lookup_noperm(name, parent);
-	if (!dentry) {
-		/*
-		 * If we know that the inode will need to be revalidated
-		 * immediately, then don't create a new dentry for it.
-		 * We'll end up doing an on the wire call either way and
-		 * this spares us an invalidation.
-		 */
-retry:
-		if (posix) {
-			switch (fattr->cf_mode & S_IFMT) {
-			case S_IFLNK:
-			case S_IFBLK:
-			case S_IFCHR:
-				reparse_need_reval = true;
-				break;
-			default:
-				break;
-			}
-		} else if (fattr->cf_cifsattrs & ATTR_REPARSE_POINT) {
+	/*
+	 * If we know that the inode will need to be revalidated
+	 * immediately, then don't create a new dentry for it.
+	 * We'll end up doing an on the wire call either way and
+	 * this spares us an invalidation.
+	 */
+	if (posix) {
+		switch (fattr->cf_mode & S_IFMT) {
+		case S_IFLNK:
+		case S_IFBLK:
+		case S_IFCHR:
 			reparse_need_reval = true;
+			break;
+		default:
+			break;
 		}
+	} else if (fattr->cf_cifsattrs & ATTR_REPARSE_POINT) {
+		reparse_need_reval = true;
+	}
 
-		if (reparse_need_reval ||
-		    (fattr->cf_flags & CIFS_FATTR_NEED_REVAL))
-			return;
+retry:
+	if (reparse_need_reval ||
+	    (fattr->cf_flags & CIFS_FATTR_NEED_REVAL))
+		dentry = try_lookup_noperm(name, parent);
+	else
+		dentry = d_alloc_trylock(parent, name);
 
-		dentry = d_alloc_parallel(parent, name);
-	}
-	if (IS_ERR(dentry))
+	if (!dentry || IS_ERR(dentry))
 		return;
 	if (!d_in_lookup(dentry)) {
 		inode = d_inode(dentry);
-- 
2.50.0.107.gf914562f5916.dirty


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 0/3] smb/client(cifs): prepare for VFS locking changes.
  2026-09-29  2:40 [PATCH v2 0/3] smb/client(cifs): prepare for VFS locking changes NeilBrown
                   ` (2 preceding siblings ...)
  2026-09-29  2:40 ` [PATCH v2 3/3] smb/client: Use d_alloc_trylock() in cifs_prime_dcache() NeilBrown
@ 2026-10-21 13:43 ` Namjae Jeon
  2026-10-23  0:03 ` Paulo Alcantara
  4 siblings, 0 replies; 8+ messages in thread
From: Namjae Jeon @ 2026-10-21 13:43 UTC (permalink / raw)
  To: NeilBrown
  Cc: Paulo Alcantara, Alexander Viro, Christian Brauner, Jeff Layton,
	Jan Kara, linux-fsdevel, linux-cifs, linux-kernel

On Tue, Sep 29, 2026 at 11:44 AM NeilBrown <neilb@ownmail.net> wrote:
>
> The previous version of this series had a couple of problems reported by sashiko,
> both in 2/3.
> 1/ an error from cifs_lookup() wasn't handled properly - it should be fatal
> 2/ The d_is_positive() tested the wrong dentry.
>
> Original intro message below.
> Thanks,
> NeilBrown
>
>
> I am working on changes to directory locking.  The medium term goal is
> to lift d_alloc_parallel() locking above i_rwsem, so d_alloc_paralle()
> can be run unlocked, and consequently cannot be called when ->i_rwsem is
> held.
>
> The requires:
>  1/ that d_alloc_parallel() not be called while i_rwsem is held.  cifs
>     does this in a couple of places.  A new interface d_alloc_trylock()
>     is now available and can be used instead.
>
>  2/ a dentry must not be d_drop()ed while an operation is ongoing, as an
>     unlocked d_alloc_parallel() would then be able to create a new
>     dentry with the same name.  In particular d_rehash() must be phased
>     out as it won't be safe.  This particularly affects the mechanism to
>     block lookups/revalidates while an unlink/rename is happening.  The
>     mechanism is changed to use one similar to how NFS addresses this
>     needed, though using the new LOOKUP_PRIVATE flag.
>
> The patches depend on patches recently added to the vfs tree.  I would
> prefer these land in that tree too with an Acked-by or similar from cifs
> maintainers.
>
> Thanks,
> NeilBrown
>
>  [PATCH v2 1/3] smb/client: don't unhash and rehash to prevent new
>  [PATCH v2 2/3] smb/client: use d_alloc_trylock in
>  [PATCH v2 3/3] smb/client: Use d_alloc_trylock() in
Looks good to me:)
Acked-by: Namjae Jeon <linkinjeon@kernel.org>
Thanks!

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 3/3] smb/client: Use d_alloc_trylock() in cifs_prime_dcache()
  2026-09-29  2:40 ` [PATCH v2 3/3] smb/client: Use d_alloc_trylock() in cifs_prime_dcache() NeilBrown
@ 2026-10-23  0:02   ` Paulo Alcantara
  2026-10-23  1:32     ` NeilBrown
  0 siblings, 1 reply; 8+ messages in thread
From: Paulo Alcantara @ 2026-10-23  0:02 UTC (permalink / raw)
  To: NeilBrown, Namjae Jeon, Alexander Viro, Christian Brauner
  Cc: Jeff Layton, Jan Kara, linux-fsdevel, linux-cifs, linux-kernel

NeilBrown <neilb@ownmail.net> writes:

> @@ -77,36 +77,34 @@ cifs_prime_dcache(struct dentry *parent, struct qstr *name,
>  
>  	cifs_dbg(FYI, "%s: for %s\n", __func__, name->name);
>  
> -	dentry = try_lookup_noperm(name, parent);
> -	if (!dentry) {
> -		/*
> -		 * If we know that the inode will need to be revalidated
> -		 * immediately, then don't create a new dentry for it.
> -		 * We'll end up doing an on the wire call either way and
> -		 * this spares us an invalidation.
> -		 */
> -retry:
> -		if (posix) {
> -			switch (fattr->cf_mode & S_IFMT) {
> -			case S_IFLNK:
> -			case S_IFBLK:
> -			case S_IFCHR:
> -				reparse_need_reval = true;
> -				break;
> -			default:
> -				break;
> -			}
> -		} else if (fattr->cf_cifsattrs & ATTR_REPARSE_POINT) {
> +	/*
> +	 * If we know that the inode will need to be revalidated
> +	 * immediately, then don't create a new dentry for it.
> +	 * We'll end up doing an on the wire call either way and
> +	 * this spares us an invalidation.
> +	 */
> +	if (posix) {
> +		switch (fattr->cf_mode & S_IFMT) {
> +		case S_IFLNK:
> +		case S_IFBLK:
> +		case S_IFCHR:
>  			reparse_need_reval = true;
> +			break;
> +		default:
> +			break;
>  		}
> +	} else if (fattr->cf_cifsattrs & ATTR_REPARSE_POINT) {
> +		reparse_need_reval = true;
> +	}
>  
> -		if (reparse_need_reval ||
> -		    (fattr->cf_flags & CIFS_FATTR_NEED_REVAL))
> -			return;
> +retry:
> +	if (reparse_need_reval ||
> +	    (fattr->cf_flags & CIFS_FATTR_NEED_REVAL))
> +		dentry = try_lookup_noperm(name, parent);
> +	else
> +		dentry = d_alloc_trylock(parent, name);
>  
> -		dentry = d_alloc_parallel(parent, name);
> -	}
> -	if (IS_ERR(dentry))
> +	if (!dentry || IS_ERR(dentry))

nit: IS_ERR_OR_NULL(dentry)

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 0/3] smb/client(cifs): prepare for VFS locking changes.
  2026-09-29  2:40 [PATCH v2 0/3] smb/client(cifs): prepare for VFS locking changes NeilBrown
                   ` (3 preceding siblings ...)
  2026-10-21 13:43 ` [PATCH v2 0/3] smb/client(cifs): prepare for VFS locking changes Namjae Jeon
@ 2026-10-23  0:03 ` Paulo Alcantara
  4 siblings, 0 replies; 8+ messages in thread
From: Paulo Alcantara @ 2026-10-23  0:03 UTC (permalink / raw)
  To: NeilBrown, Namjae Jeon, Alexander Viro, Christian Brauner
  Cc: Jeff Layton, Jan Kara, linux-fsdevel, linux-cifs, linux-kernel

NeilBrown <neilb@ownmail.net> writes:

> The previous version of this series had a couple of problems reported by sashiko,
> both in 2/3.
> 1/ an error from cifs_lookup() wasn't handled properly - it should be fatal
> 2/ The d_is_positive() tested the wrong dentry.
>
> Original intro message below.
> Thanks,
> NeilBrown
>
>
> I am working on changes to directory locking.  The medium term goal is
> to lift d_alloc_parallel() locking above i_rwsem, so d_alloc_paralle()
> can be run unlocked, and consequently cannot be called when ->i_rwsem is
> held.
>
> The requires:
>  1/ that d_alloc_parallel() not be called while i_rwsem is held.  cifs
>     does this in a couple of places.  A new interface d_alloc_trylock()
>     is now available and can be used instead.
>
>  2/ a dentry must not be d_drop()ed while an operation is ongoing, as an
>     unlocked d_alloc_parallel() would then be able to create a new
>     dentry with the same name.  In particular d_rehash() must be phased
>     out as it won't be safe.  This particularly affects the mechanism to
>     block lookups/revalidates while an unlink/rename is happening.  The
>     mechanism is changed to use one similar to how NFS addresses this
>     needed, though using the new LOOKUP_PRIVATE flag.
>
> The patches depend on patches recently added to the vfs tree.  I would
> prefer these land in that tree too with an Acked-by or similar from cifs
> maintainers.
> ...

LGTM.  Thanks Neil.

Acked-by: Paulo Alcantara <pc@manguebit.org>

Christian, feel free take it through vfs tree.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 3/3] smb/client: Use d_alloc_trylock() in cifs_prime_dcache()
  2026-10-23  0:02   ` Paulo Alcantara
@ 2026-10-23  1:32     ` NeilBrown
  0 siblings, 0 replies; 8+ messages in thread
From: NeilBrown @ 2026-10-23  1:32 UTC (permalink / raw)
  To: Paulo Alcantara
  Cc: Namjae Jeon, Alexander Viro, Christian Brauner, Jeff Layton,
	Jan Kara, linux-fsdevel, linux-cifs, linux-kernel

On Fri, 23 Oct 2026, Paulo Alcantara wrote:
> NeilBrown <neilb@ownmail.net> writes:
> 
> > @@ -77,36 +77,34 @@ cifs_prime_dcache(struct dentry *parent, struct qstr *name,
> >  
> >  	cifs_dbg(FYI, "%s: for %s\n", __func__, name->name);
> >  
> > -	dentry = try_lookup_noperm(name, parent);
> > -	if (!dentry) {
> > -		/*
> > -		 * If we know that the inode will need to be revalidated
> > -		 * immediately, then don't create a new dentry for it.
> > -		 * We'll end up doing an on the wire call either way and
> > -		 * this spares us an invalidation.
> > -		 */
> > -retry:
> > -		if (posix) {
> > -			switch (fattr->cf_mode & S_IFMT) {
> > -			case S_IFLNK:
> > -			case S_IFBLK:
> > -			case S_IFCHR:
> > -				reparse_need_reval = true;
> > -				break;
> > -			default:
> > -				break;
> > -			}
> > -		} else if (fattr->cf_cifsattrs & ATTR_REPARSE_POINT) {
> > +	/*
> > +	 * If we know that the inode will need to be revalidated
> > +	 * immediately, then don't create a new dentry for it.
> > +	 * We'll end up doing an on the wire call either way and
> > +	 * this spares us an invalidation.
> > +	 */
> > +	if (posix) {
> > +		switch (fattr->cf_mode & S_IFMT) {
> > +		case S_IFLNK:
> > +		case S_IFBLK:
> > +		case S_IFCHR:
> >  			reparse_need_reval = true;
> > +			break;
> > +		default:
> > +			break;
> >  		}
> > +	} else if (fattr->cf_cifsattrs & ATTR_REPARSE_POINT) {
> > +		reparse_need_reval = true;
> > +	}
> >  
> > -		if (reparse_need_reval ||
> > -		    (fattr->cf_flags & CIFS_FATTR_NEED_REVAL))
> > -			return;
> > +retry:
> > +	if (reparse_need_reval ||
> > +	    (fattr->cf_flags & CIFS_FATTR_NEED_REVAL))
> > +		dentry = try_lookup_noperm(name, parent);
> > +	else
> > +		dentry = d_alloc_trylock(parent, name);
> >  
> > -		dentry = d_alloc_parallel(parent, name);
> > -	}
> > -	if (IS_ERR(dentry))
> > +	if (!dentry || IS_ERR(dentry))
> 
> nit: IS_ERR_OR_NULL(dentry)
> 

I'll fix that thanks.
We should really try to get rid of functions (try_lookup_noperm() in
this case) which return a NULL or an error - but that's for another day.

Thanks,
NeilBrown

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2026-10-23  1:32 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-09-29  2:40 [PATCH v2 0/3] smb/client(cifs): prepare for VFS locking changes NeilBrown
2026-09-29  2:40 ` [PATCH v2 1/3] smb/client: don't unhash and rehash to prevent new opens NeilBrown
2026-09-29  2:40 ` [PATCH v2 2/3] smb/client: use d_alloc_trylock in cifs_silly_fullpath() NeilBrown
2026-09-29  2:40 ` [PATCH v2 3/3] smb/client: Use d_alloc_trylock() in cifs_prime_dcache() NeilBrown
2026-10-23  0:02   ` Paulo Alcantara
2026-10-23  1:32     ` NeilBrown
2026-10-21 13:43 ` [PATCH v2 0/3] smb/client(cifs): prepare for VFS locking changes Namjae Jeon
2026-10-23  0:03 ` Paulo Alcantara

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox