diff mbox series

[7/9] fs/9p: rework qid2ino logic

Message ID 20240106-ericvh-fix-cache-dups-v1-7-538c2074f363@kernel.org (mailing list archive)
State New
Headers show
Series fs/9p: simplify inode lookup operations | expand

Commit Message

Eric Van Hensbergen Jan. 6, 2024, 2:11 a.m. UTC 
This changes from a function to a macro because we can
figure out if we are 32 or 64 bit at compile time.

Signed-off-by: Eric Van Hensbergen <ericvh@kernel.org>
---
 fs/9p/v9fs_vfs.h       |  7 ++++++-
 fs/9p/vfs_dir.c        |  6 +++---
 fs/9p/vfs_inode.c      | 31 +------------------------------
 fs/9p/vfs_inode_dotl.c |  6 ++----
 4 files changed, 12 insertions(+), 38 deletions(-)

Comments

Dominique Martinet Jan. 8, 2024, 11:28 a.m. UTC | #1 
Eric Van Hensbergen wrote on Sat, Jan 06, 2024 at 02:11:14AM +0000:
> This changes from a function to a macro because we can
> figure out if we are 32 or 64 bit at compile time.
> 
> Signed-off-by: Eric Van Hensbergen <ericvh@kernel.org>

Couple of remarks on this one.
patches 2-6 so far all looked good to me.


> ---
>  fs/9p/v9fs_vfs.h       |  7 ++++++-
>  fs/9p/vfs_dir.c        |  6 +++---
>  fs/9p/vfs_inode.c      | 31 +------------------------------
>  fs/9p/vfs_inode_dotl.c |  6 ++----
>  4 files changed, 12 insertions(+), 38 deletions(-)
> 
> diff --git a/fs/9p/v9fs_vfs.h b/fs/9p/v9fs_vfs.h
> index ad0310deb6c8..789e1188d5dc 100644
> --- a/fs/9p/v9fs_vfs.h
> +++ b/fs/9p/v9fs_vfs.h
> @@ -43,7 +43,12 @@ void v9fs_free_inode(struct inode *inode);
>  int v9fs_init_inode(struct v9fs_session_info *v9ses,
>  		    struct inode *inode, umode_t mode, dev_t rdev);
>  void v9fs_evict_inode(struct inode *inode);
> -ino_t v9fs_qid2ino(struct p9_qid *qid);
> +#if (ULONG_MAX == 0xffffffffUL)

The standard preprocessor condition in the kernel for this is checking
BITS_PER_LONG, e.g.
include/asm-generic/bitops/fls64.h
18:#if BITS_PER_LONG == 32
26:#elif BITS_PER_LONG == 64
33:#else
34:#error BITS_PER_LONG not 32 or 64
35:#endif

> +#define QID2INO(q) (ino_t) (((q)->path+2) ^ (((q)->path) >> 32))
> +#else
> +#define QID2INO(q) (ino_t) ((q)->path+2)
> +#endif
> +
>  void v9fs_stat2inode(struct p9_wstat *stat, struct inode *inode,
>  		      struct super_block *sb, unsigned int flags);
>  void v9fs_stat2inode_dotl(struct p9_stat_dotl *stat, struct inode *inode,
> diff --git a/fs/9p/vfs_dir.c b/fs/9p/vfs_dir.c
> index 4102759a5cb5..13a53da75ec8 100644
> --- a/fs/9p/vfs_dir.c
> +++ b/fs/9p/vfs_dir.c
> @@ -125,9 +125,9 @@ static int v9fs_dir_readdir(struct file *file, struct dir_context *ctx)
>  				p9_debug(P9_DEBUG_VFS, "returned %d\n", err);
>  				return -EIO;
>  			}
> -
> +

stray white space?

>  			over = !dir_emit(ctx, st.name, strlen(st.name),
> -					 v9fs_qid2ino(&st.qid), dt_type(&st));
> +					QID2INO(&st.qid), dt_type(&st));
>  			p9stat_free(&st);
>  			if (over)
>  				return 0;
> @@ -184,7 +184,7 @@ static int v9fs_dir_readdir_dotl(struct file *file, struct dir_context *ctx)
>  
>  			if (!dir_emit(ctx, curdirent.d_name,
>  				      strlen(curdirent.d_name),
> -				      v9fs_qid2ino(&curdirent.qid),
> +				      QID2INO(&curdirent.qid),
>  				      curdirent.d_type))
>  				return 0;
>  
> diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
> index 02761009946b..fe8cbcdf4b5f 100644
> --- a/fs/9p/vfs_inode.c
> +++ b/fs/9p/vfs_inode.c
> @@ -407,7 +407,6 @@ static struct inode *v9fs_qid_iget(struct super_block *sb,
>  	dev_t rdev;
>  	int retval;
>  	umode_t umode;
> -	unsigned long i_ino;
>  	struct inode *inode;
>  	struct v9fs_session_info *v9ses = sb->s_fs_info;
>  	int (*test)(struct inode *inode, void *data);
> @@ -417,8 +416,7 @@ static struct inode *v9fs_qid_iget(struct super_block *sb,
>  	else
>  		test = v9fs_test_inode;
>  
> -	i_ino = v9fs_qid2ino(qid);
> -	inode = iget5_locked(sb, i_ino, test, v9fs_set_inode, st);
> +	inode = iget5_locked(sb, QID2INO(qid), test, v9fs_set_inode, st);
>  	if (!inode)
>  		return ERR_PTR(-ENOMEM);
>  	if (!(inode->i_state & I_NEW))
> @@ -428,7 +426,6 @@ static struct inode *v9fs_qid_iget(struct super_block *sb,
>  	 * FIXME!! we may need support for stale inodes
>  	 * later.
>  	 */
> -	inode->i_ino = i_ino;

This is gone, but v9fs_qid_iget_dotl() still sets i_ino -- didn't check
which is needed but we probably want to stay coherent between the two.

>  	umode = p9mode2unixmode(v9ses, st, &rdev);
>  	retval = v9fs_init_inode(v9ses, inode, umode, rdev);
>  	if (retval)
> @@ -1159,32 +1156,6 @@ v9fs_stat2inode(struct p9_wstat *stat, struct inode *inode,
>  	v9inode->cache_validity &= ~V9FS_INO_INVALID_ATTR;
>  }
>  
> -/**
> - * v9fs_qid2ino - convert qid into inode number
> - * @qid: qid to hash
> - *
> - *  We add 2 to qid->path to keep reserved inode
> - *  numbers reserved.
> - * 
> - *  Its possible in the future we will make qid->path
> - *  just a hash to lookup inodes, but this breaks too
> - *  much right now.
> - * 
> - */
> -
> -ino_t v9fs_qid2ino(struct p9_qid *qid)
> -{
> -	ino_t i = 0;
> -
> -	WARN_ON((qid->path+2)==0);
> -
> -	if (sizeof(ino_t) < sizeof(qid->path))
> -		i = (ino_t) ((qid->path+2) ^ (qid->path >> 32));
> -	else
> -		i = (ino_t) qid->path;
> -
> -	return i;
> -}
>  
>  /**
>   * v9fs_vfs_get_link - follow a symlink path
> diff --git a/fs/9p/vfs_inode_dotl.c b/fs/9p/vfs_inode_dotl.c
> index 8eee13bae887..2699d7b3b8e8 100644
> --- a/fs/9p/vfs_inode_dotl.c
> +++ b/fs/9p/vfs_inode_dotl.c
> @@ -100,7 +100,6 @@ static struct inode *v9fs_qid_iget_dotl(struct super_block *sb,
>  					int new)
>  {
>  	int retval;
> -	unsigned long i_ino;
>  	struct inode *inode;
>  	struct v9fs_session_info *v9ses = sb->s_fs_info;
>  	int (*test)(struct inode *inode, void *data);
> @@ -110,8 +109,7 @@ static struct inode *v9fs_qid_iget_dotl(struct super_block *sb,
>  	else
>  		test = v9fs_test_inode_dotl;
>  
> -	i_ino = v9fs_qid2ino(qid);
> -	inode = iget5_locked(sb, i_ino, test, v9fs_set_inode_dotl, st);
> +	inode = iget5_locked(sb, QID2INO(qid), test, v9fs_set_inode_dotl, st);
>  	if (!inode)
>  		return ERR_PTR(-ENOMEM);
>  	if (!(inode->i_state & I_NEW))
> @@ -121,7 +119,7 @@ static struct inode *v9fs_qid_iget_dotl(struct super_block *sb,
>  	 * FIXME!! we may need support for stale inodes
>  	 * later.
>  	 */
> -	inode->i_ino = i_ino;
> +	inode->i_ino = QID2INO(qid);
>  	retval = v9fs_init_inode(v9ses, inode,
>  				 st->st_mode, new_decode_dev(st->st_rdev));
>  	if (retval)
>
Eric Van Hensbergen Jan. 8, 2024, 12:56 p.m. UTC | #2 
On Mon, Jan 08, 2024 at 08:28:15PM +0900, asmadeus@codewreck.org wrote:
> Eric Van Hensbergen wrote on Sat, Jan 06, 2024 at 02:11:14AM +0000:
> > 
> > diff --git a/fs/9p/v9fs_vfs.h b/fs/9p/v9fs_vfs.h
> > index ad0310deb6c8..789e1188d5dc 100644
> > --- a/fs/9p/v9fs_vfs.h
> > +++ b/fs/9p/v9fs_vfs.h
> > @@ -43,7 +43,12 @@ void v9fs_free_inode(struct inode *inode);
> >  int v9fs_init_inode(struct v9fs_session_info *v9ses,
> >  		    struct inode *inode, umode_t mode, dev_t rdev);
> >  void v9fs_evict_inode(struct inode *inode);
> > -ino_t v9fs_qid2ino(struct p9_qid *qid);
> > +#if (ULONG_MAX == 0xffffffffUL)
> 
> The standard preprocessor condition in the kernel for this is checking
> BITS_PER_LONG, e.g.
> include/asm-generic/bitops/fls64.h
> 18:#if BITS_PER_LONG == 32
> 26:#elif BITS_PER_LONG == 64
> 33:#else
> 34:#error BITS_PER_LONG not 32 or 64
> 35:#endif
>

Thanks for that, will update to be consistent with rest of kernel.
 
> > -
> > +
> 
> stray white space?
> 

oops, will fix.

> > @@ -428,7 +426,6 @@ static struct inode *v9fs_qid_iget(struct super_block *sb,
> >  	 * FIXME!! we may need support for stale inodes
> >  	 * later.
> >  	 */
> > -	inode->i_ino = i_ino;
> 
> This is gone, but v9fs_qid_iget_dotl() still sets i_ino -- didn't check
> which is needed but we probably want to stay coherent between the two.
>

Good catch, deleted it from .L (and later here but in a different patch)
because its largely vestigial since this gets set in iget_locked anyways.
Will fix up to be more consistent.

      -eric
diff mbox series

Patch

diff --git a/fs/9p/v9fs_vfs.h b/fs/9p/v9fs_vfs.h
index ad0310deb6c8..789e1188d5dc 100644
--- a/fs/9p/v9fs_vfs.h
+++ b/fs/9p/v9fs_vfs.h
@@ -43,7 +43,12 @@  void v9fs_free_inode(struct inode *inode);
 int v9fs_init_inode(struct v9fs_session_info *v9ses,
 		    struct inode *inode, umode_t mode, dev_t rdev);
 void v9fs_evict_inode(struct inode *inode);
-ino_t v9fs_qid2ino(struct p9_qid *qid);
+#if (ULONG_MAX == 0xffffffffUL)
+#define QID2INO(q) (ino_t) (((q)->path+2) ^ (((q)->path) >> 32))
+#else
+#define QID2INO(q) (ino_t) ((q)->path+2)
+#endif
+
 void v9fs_stat2inode(struct p9_wstat *stat, struct inode *inode,
 		      struct super_block *sb, unsigned int flags);
 void v9fs_stat2inode_dotl(struct p9_stat_dotl *stat, struct inode *inode,
diff --git a/fs/9p/vfs_dir.c b/fs/9p/vfs_dir.c
index 4102759a5cb5..13a53da75ec8 100644
--- a/fs/9p/vfs_dir.c
+++ b/fs/9p/vfs_dir.c
@@ -125,9 +125,9 @@  static int v9fs_dir_readdir(struct file *file, struct dir_context *ctx)
 				p9_debug(P9_DEBUG_VFS, "returned %d\n", err);
 				return -EIO;
 			}
-
+			
 			over = !dir_emit(ctx, st.name, strlen(st.name),
-					 v9fs_qid2ino(&st.qid), dt_type(&st));
+					QID2INO(&st.qid), dt_type(&st));
 			p9stat_free(&st);
 			if (over)
 				return 0;
@@ -184,7 +184,7 @@  static int v9fs_dir_readdir_dotl(struct file *file, struct dir_context *ctx)
 
 			if (!dir_emit(ctx, curdirent.d_name,
 				      strlen(curdirent.d_name),
-				      v9fs_qid2ino(&curdirent.qid),
+				      QID2INO(&curdirent.qid),
 				      curdirent.d_type))
 				return 0;
 
diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
index 02761009946b..fe8cbcdf4b5f 100644
--- a/fs/9p/vfs_inode.c
+++ b/fs/9p/vfs_inode.c
@@ -407,7 +407,6 @@  static struct inode *v9fs_qid_iget(struct super_block *sb,
 	dev_t rdev;
 	int retval;
 	umode_t umode;
-	unsigned long i_ino;
 	struct inode *inode;
 	struct v9fs_session_info *v9ses = sb->s_fs_info;
 	int (*test)(struct inode *inode, void *data);
@@ -417,8 +416,7 @@  static struct inode *v9fs_qid_iget(struct super_block *sb,
 	else
 		test = v9fs_test_inode;
 
-	i_ino = v9fs_qid2ino(qid);
-	inode = iget5_locked(sb, i_ino, test, v9fs_set_inode, st);
+	inode = iget5_locked(sb, QID2INO(qid), test, v9fs_set_inode, st);
 	if (!inode)
 		return ERR_PTR(-ENOMEM);
 	if (!(inode->i_state & I_NEW))
@@ -428,7 +426,6 @@  static struct inode *v9fs_qid_iget(struct super_block *sb,
 	 * FIXME!! we may need support for stale inodes
 	 * later.
 	 */
-	inode->i_ino = i_ino;
 	umode = p9mode2unixmode(v9ses, st, &rdev);
 	retval = v9fs_init_inode(v9ses, inode, umode, rdev);
 	if (retval)
@@ -1159,32 +1156,6 @@  v9fs_stat2inode(struct p9_wstat *stat, struct inode *inode,
 	v9inode->cache_validity &= ~V9FS_INO_INVALID_ATTR;
 }
 
-/**
- * v9fs_qid2ino - convert qid into inode number
- * @qid: qid to hash
- *
- *  We add 2 to qid->path to keep reserved inode
- *  numbers reserved.
- * 
- *  Its possible in the future we will make qid->path
- *  just a hash to lookup inodes, but this breaks too
- *  much right now.
- * 
- */
-
-ino_t v9fs_qid2ino(struct p9_qid *qid)
-{
-	ino_t i = 0;
-
-	WARN_ON((qid->path+2)==0);
-
-	if (sizeof(ino_t) < sizeof(qid->path))
-		i = (ino_t) ((qid->path+2) ^ (qid->path >> 32));
-	else
-		i = (ino_t) qid->path;
-
-	return i;
-}
 
 /**
  * v9fs_vfs_get_link - follow a symlink path
diff --git a/fs/9p/vfs_inode_dotl.c b/fs/9p/vfs_inode_dotl.c
index 8eee13bae887..2699d7b3b8e8 100644
--- a/fs/9p/vfs_inode_dotl.c
+++ b/fs/9p/vfs_inode_dotl.c
@@ -100,7 +100,6 @@  static struct inode *v9fs_qid_iget_dotl(struct super_block *sb,
 					int new)
 {
 	int retval;
-	unsigned long i_ino;
 	struct inode *inode;
 	struct v9fs_session_info *v9ses = sb->s_fs_info;
 	int (*test)(struct inode *inode, void *data);
@@ -110,8 +109,7 @@  static struct inode *v9fs_qid_iget_dotl(struct super_block *sb,
 	else
 		test = v9fs_test_inode_dotl;
 
-	i_ino = v9fs_qid2ino(qid);
-	inode = iget5_locked(sb, i_ino, test, v9fs_set_inode_dotl, st);
+	inode = iget5_locked(sb, QID2INO(qid), test, v9fs_set_inode_dotl, st);
 	if (!inode)
 		return ERR_PTR(-ENOMEM);
 	if (!(inode->i_state & I_NEW))
@@ -121,7 +119,7 @@  static struct inode *v9fs_qid_iget_dotl(struct super_block *sb,
 	 * FIXME!! we may need support for stale inodes
 	 * later.
 	 */
-	inode->i_ino = i_ino;
+	inode->i_ino = QID2INO(qid);
 	retval = v9fs_init_inode(v9ses, inode,
 				 st->st_mode, new_decode_dev(st->st_rdev));
 	if (retval)