Message-ID: <20250618215334.ydefakmcvfpqx4dn@pali>
Date: Wed, 18 Jun 2025 23:53:34 +0200
From: Pali Rohár <pali@...nel.org>
To: Paulo Alcantara <pc@...guebit.org>
Cc: Steve French <sfrench@...ba.org>, Tom Talpey <tom@...pey.com>,
	linux-cifs@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] cifs: Show reason why autodisabling serverino support

On Tuesday 17 June 2025 20:48:00 Paulo Alcantara wrote:
> Pali Rohár <pali@...nel.org> writes:
> 
> > On Tuesday 17 June 2025 19:23:15 Paulo Alcantara wrote:
> >> Pali Rohár <pali@...nel.org> writes:
> >> 
> >> > Extend cifs_autodisable_serverino() function to print also text message why
> >> > the function was called.
> >> >
> >> > The text message is printed just once for mount then autodisabling
> >> > serverino support. Once the serverino support is disabled for mount it will
> >> > not be re-enabled. So those text messages do not cause flooding logs.
> >> >
> >> > This change allows to debug issues why cifs.ko decide to turn off server
> >> > inode number support and hence disable support for detection of hardlinks.
> >> >
> >> > Signed-off-by: Pali Rohár <pali@...nel.org>
> >> > ---
> >> > Paulo and Tom, could you check if this change is better now for you?
> >> > It should address problems with logs flooding and also information about
> >> > harlinks (it is already printed as can be seen also in this diff).
> >> > I would like to get your ACK, so I'm trying to improve it.
> >> > ---
> >> >  fs/smb/client/cifsproto.h | 2 +-
> >> >  fs/smb/client/connect.c   | 2 +-
> >> >  fs/smb/client/dfs_cache.c | 2 +-
> >> >  fs/smb/client/inode.c     | 6 +++---
> >> >  fs/smb/client/misc.c      | 6 +++++-
> >> >  fs/smb/client/readdir.c   | 4 ++--
> >> >  6 files changed, 13 insertions(+), 9 deletions(-)
> >> >
> >> > diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h
> >> > index d550662b4e72..07a67c8c37ce 100644
> >> > --- a/fs/smb/client/cifsproto.h
> >> > +++ b/fs/smb/client/cifsproto.h
> >> > @@ -586,9 +586,9 @@ extern int cifs_do_set_acl(const unsigned int xid, struct cifs_tcon *tcon,
> >> >  			   const struct nls_table *nls_codepage, int remap);
> >> >  extern int CIFSGetExtAttr(const unsigned int xid, struct cifs_tcon *tcon,
> >> >  			const int netfid, __u64 *pExtAttrBits, __u64 *pMask);
> >> >  #endif /* CIFS_ALLOW_INSECURE_LEGACY */
> >> > -extern void cifs_autodisable_serverino(struct cifs_sb_info *cifs_sb);
> >> > +extern void cifs_autodisable_serverino(struct cifs_sb_info *cifs_sb, const char *reason, int rc);
> >> >  extern bool couldbe_mf_symlink(const struct cifs_fattr *fattr);
> >> >  extern int check_mf_symlink(unsigned int xid, struct cifs_tcon *tcon,
> >> >  			      struct cifs_sb_info *cifs_sb,
> >> >  			      struct cifs_fattr *fattr,
> >> > diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
> >> > index 6bf04d9a5491..819721dfd5bb 100644
> >> > --- a/fs/smb/client/connect.c
> >> > +++ b/fs/smb/client/connect.c
> >> > @@ -3907,9 +3907,9 @@ int cifs_mount(struct cifs_sb_info *cifs_sb, struct smb3_fs_context *ctx)
> >> >  	/*
> >> >  	 * After reconnecting to a different server, unique ids won't match anymore, so we disable
> >> >  	 * serverino. This prevents dentry revalidation to think the dentry are stale (ESTALE).
> >> >  	 */
> >> > -	cifs_autodisable_serverino(cifs_sb);
> >> > +	cifs_autodisable_serverino(cifs_sb, "Reconnecting to different server, inode numbers won't match anymore", 0);
> >> 
> >> We are mounting an DFS share, not reconnecting.  The message is
> >> misleading.
> >
> > I mostly copied the comment above the cifs_autodisable_serverino() call.
> > Does it mean that the existing comment about reconnecting is wrong too?
> 
> The comment is trying to say why it disabled 'serverino'.  DFS failover
> may potentially connect to a different server and share, hence the inode
> numbers will no longer be valid.  The function is also called
> cifs_mount().

Ok. What do you suggest to put into the message? It would be good to not
have it too long, but also to be useful.

> >> >  	/*
> >> >  	 * Force the use of prefix path to support failover on DFS paths that resolve to targets
> >> >  	 * that have different prefix paths.
> >> >  	 */
> >> > diff --git a/fs/smb/client/dfs_cache.c b/fs/smb/client/dfs_cache.c
> >> > index 4dada26d56b5..c3fe85c31e2b 100644
> >> > --- a/fs/smb/client/dfs_cache.c
> >> > +++ b/fs/smb/client/dfs_cache.c
> >> > @@ -1288,9 +1288,9 @@ int dfs_cache_remount_fs(struct cifs_sb_info *cifs_sb)
> >> >  	/*
> >> >  	 * After reconnecting to a different server, unique ids won't match anymore, so we disable
> >> >  	 * serverino. This prevents dentry revalidation to think the dentry are stale (ESTALE).
> >> >  	 */
> >> > -	cifs_autodisable_serverino(cifs_sb);
> >> > +	cifs_autodisable_serverino(cifs_sb, "Reconnecting to different server, inode numbers won't match anymore", 0);
> >> 
> >> Ditto.
> >> 
> >> >  	/*
> >> >  	 * Force the use of prefix path to support failover on DFS paths that resolve to targets
> >> >  	 * that have different prefix paths.
> >> >  	 */
> >> > diff --git a/fs/smb/client/inode.c b/fs/smb/client/inode.c
> >> > index cd06598eacbd..b1c6e3986278 100644
> >> > --- a/fs/smb/client/inode.c
> >> > +++ b/fs/smb/client/inode.c
> >> > @@ -1076,9 +1076,9 @@ static void cifs_set_fattr_ino(int xid, struct cifs_tcon *tcon, struct super_blo
> >> >  		if (*inode)
> >> >  			fattr->cf_uniqueid = CIFS_I(*inode)->uniqueid;
> >> >  		else {
> >> >  			fattr->cf_uniqueid = iunique(sb, ROOT_I);
> >> > -			cifs_autodisable_serverino(cifs_sb);
> >> > +			cifs_autodisable_serverino(cifs_sb, "Cannot retrieve inode number via get_srv_inum", rc);
> >> 
> >> Looks good.
> >> 
> >> >  		}
> >> >  		return;
> >> >  	}
> >> >  
> >> > @@ -1529,9 +1529,9 @@ cifs_iget(struct super_block *sb, struct cifs_fattr *fattr)
> >> >  		if (fattr->cf_flags & CIFS_FATTR_INO_COLLISION) {
> >> >  			fattr->cf_flags &= ~CIFS_FATTR_INO_COLLISION;
> >> >  
> >> >  			if (inode_has_hashed_dentries(inode)) {
> >> > -				cifs_autodisable_serverino(CIFS_SB(sb));
> >> > +				cifs_autodisable_serverino(CIFS_SB(sb), "Inode number collision detected", 0);
> >> 
> >> Looks good.
> >> 
> >> >  				iput(inode);
> >> >  				fattr->cf_uniqueid = iunique(sb, ROOT_I);
> >> >  				goto retry_iget5_locked;
> >> >  			}
> >> > @@ -1596,9 +1596,9 @@ struct inode *cifs_root_iget(struct super_block *sb)
> >> >  iget_root:
> >> >  	if (!rc) {
> >> >  		if (fattr.cf_flags & CIFS_FATTR_JUNCTION) {
> >> >  			fattr.cf_flags &= ~CIFS_FATTR_JUNCTION;
> >> > -			cifs_autodisable_serverino(cifs_sb);
> >> > +			cifs_autodisable_serverino(cifs_sb, "Cannot retrieve attributes for junction point", rc);
> >> 
> >> This has nothing to do with not being able to retrieve attributes.  It
> >> is simply disabling 'serverino' to prevent inode collisions with
> >> surrogate reparse points (automounts).  This should also printed with
> >> FYI.
> >
> > Ok. So then I misunderstood the code around. Do you know when exactly
> > can this case happen? And it is really a problem? Because name surrogate
> > reparse point already creates a new mount hierarchy for which is
> > generated new st_dev major and minor numbers and hence inode collisions
> > should not happen (they do not share st_dev anymore).
> 
> There was a bug report of someone having inode collisions in a share
> that had a reparse mount point, so the server was returning duplicate
> inode numbers for files inside that share.  That is why we set those
> directories as automounts and then disable 'serverino' only for them, so
> the parent mount can still rely on the inode numbers from the server and
> having hardlinks working.
> 
> Note that disabling 'serverino' means that the client won't trust the
> inode numbers from the server and it will generate its own inode
> numbers.  I don't understand why st_dev is relevant here.

Different st_dev values means that they are different filesystem / mounts
So e.g. /dev/ has different st_dev value than the /mnt/smb. And hence
the same inode number can be in /dev/ and /mnt/smb, and they will not
conflict.

And same applies for name surrogate reparse points. Linux for more major
versions is reporting different st_dev value for name surrogate reparse
points, so their inode number would not conflict with the parent / main
mount point.

> >
> >> >  		}
> >> >  		inode = cifs_iget(sb, &fattr);
> >> >  	}
> >> >  
> >> > diff --git a/fs/smb/client/misc.c b/fs/smb/client/misc.c
> >> > index e77017f47084..409277883e8a 100644
> >> > --- a/fs/smb/client/misc.c
> >> > +++ b/fs/smb/client/misc.c
> >> > @@ -551,9 +551,9 @@ dump_smb(void *buf, int smb_buf_length)
> >> >  		       smb_buf_length, true);
> >> >  }
> >> >  
> >> >  void
> >> > -cifs_autodisable_serverino(struct cifs_sb_info *cifs_sb)
> >> > +cifs_autodisable_serverino(struct cifs_sb_info *cifs_sb, const char *reason, int rc)
> >> >  {
> >> >  	if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SERVER_INUM) {
> >> >  		struct cifs_tcon *tcon = NULL;
> >> >  
> >> > @@ -561,8 +561,12 @@ cifs_autodisable_serverino(struct cifs_sb_info *cifs_sb)
> >> >  			tcon = cifs_sb_master_tcon(cifs_sb);
> >> >  
> >> >  		cifs_sb->mnt_cifs_flags &= ~CIFS_MOUNT_SERVER_INUM;
> >> >  		cifs_sb->mnt_cifs_serverino_autodisabled = true;
> >> > +		if (rc)
> >> > +			cifs_dbg(VFS, "%s: %d\n", reason, rc);
> >> > +		else
> >> > +			cifs_dbg(VFS, "%s\n", reason);
> >> >  		cifs_dbg(VFS, "Autodisabling the use of server inode numbers on %s\n",
> >> >  			 tcon ? tcon->tree_name : "new server");
> >> >  		cifs_dbg(VFS, "The server doesn't seem to support them properly or the files might be on different servers (DFS)\n");
> >> >  		cifs_dbg(VFS, "Hardlinks will not be recognized on this mount. Consider mounting with the \"noserverino\" option to silence this message.\n");
> >> > diff --git a/fs/smb/client/readdir.c b/fs/smb/client/readdir.c
> >> > index 787d6bcb5d1d..06e90921f751 100644
> >> > --- a/fs/smb/client/readdir.c
> >> > +++ b/fs/smb/client/readdir.c
> >> > @@ -412,9 +412,9 @@ _initiate_cifs_search(const unsigned int xid, struct file *file,
> >> >  	if (rc == 0) {
> >> >  		cifsFile->invalidHandle = false;
> >> >  	} else if ((rc == -EOPNOTSUPP) &&
> >> >  		   (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SERVER_INUM)) {
> >> > -		cifs_autodisable_serverino(cifs_sb);
> >> > +		cifs_autodisable_serverino(cifs_sb, "Cannot retrieve inode number via query_dir_first", rc);
> >> 
> >> Looks good.
> >> 
> >> >  		goto ffirst_retry;
> >> >  	}
> >> >  error_exit:
> >> >  	cifs_put_tlink(tlink);
> >> > @@ -1006,9 +1006,9 @@ static int cifs_filldir(char *find_entry, struct file *file,
> >> >  	if (de.ino && (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SERVER_INUM)) {
> >> >  		fattr.cf_uniqueid = de.ino;
> >> >  	} else {
> >> >  		fattr.cf_uniqueid = iunique(sb, ROOT_I);
> >> > -		cifs_autodisable_serverino(cifs_sb);
> >> > +		cifs_autodisable_serverino(cifs_sb, "Cannot retrieve inode number", 0);
> >> 
> >> Perhaps also mention which function it wasn't able to retrieve inode
> >> number from like above?
> >
> > I quickly look at this code around and I was not able to figure out what
> > is that function which was not able to retrieve inode. So I did not
> > write it into the message. Do you know, or could you figure out what is
> > that function / callback?
> 
> "Cannot retrieve inode number from readdir"

Ok. But what is the callback which caused this issue for readdir here?
In the previous one, it was server->ops->query_dir_first.

Powered by blists - more mailing lists