diff mbox series

[v4,2/3] fs/ufs: Change the signature of ufs_get_page()

Message ID 20221221172802.18743-3-fmdefrancesco@gmail.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series fs/ufs: Replace kmap() with kmap_local_page | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch

Commit Message

Fabio M. De Francesco Dec. 21, 2022, 5:28 p.m. UTC 
Change the signature of ufs_get_page() in order to prepare this function
to the conversion to the use of kmap_local_page(). Change also those call
sites which are required to conform its invocations to the new
signature.

Cc: Ira Weiny <ira.weiny@intel.com>
Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
---
 fs/ufs/dir.c | 49 +++++++++++++++++++++----------------------------
 1 file changed, 21 insertions(+), 28 deletions(-)

Comments

Ira Weiny Dec. 22, 2022, 5:13 a.m. UTC | #1 
On Wed, Dec 21, 2022 at 06:28:01PM +0100, Fabio M. De Francesco wrote:
> Change the signature of ufs_get_page() in order to prepare this function
> to the conversion to the use of kmap_local_page(). Change also those call
> sites which are required to conform its invocations to the new
> signature.
> 
> Cc: Ira Weiny <ira.weiny@intel.com>
> Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> ---
>  fs/ufs/dir.c | 49 +++++++++++++++++++++----------------------------
>  1 file changed, 21 insertions(+), 28 deletions(-)
> 
> diff --git a/fs/ufs/dir.c b/fs/ufs/dir.c
> index 69f78583c9c1..9fa86614d2d1 100644
> --- a/fs/ufs/dir.c
> +++ b/fs/ufs/dir.c
> @@ -185,7 +185,7 @@ static bool ufs_check_page(struct page *page)
>  	return false;
>  }
>  
> -static struct page *ufs_get_page(struct inode *dir, unsigned long n)
> +static void *ufs_get_page(struct inode *dir, unsigned long n, struct page **p)
>  {
>  	struct address_space *mapping = dir->i_mapping;
>  	struct page *page = read_mapping_page(mapping, n, NULL);
> @@ -195,8 +195,10 @@ static struct page *ufs_get_page(struct inode *dir, unsigned long n)
>  			if (!ufs_check_page(page))
>  				goto fail;
>  		}
> +		*p = page;
> +		return page_address(page);
>  	}
> -	return page;
> +	return ERR_CAST(page);
>  
>  fail:
>  	ufs_put_page(page);
> @@ -227,15 +229,12 @@ ufs_next_entry(struct super_block *sb, struct ufs_dir_entry *p)
>  
>  struct ufs_dir_entry *ufs_dotdot(struct inode *dir, struct page **p)
>  {
> -	struct page *page = ufs_get_page(dir, 0);
> -	struct ufs_dir_entry *de = NULL;
> +	struct ufs_dir_entry *de = ufs_get_page(dir, 0, p);

I don't know why but ufs_get_page() returning an address read really odd to me.
But rolling around my head alternative names nothing seems better than this.

>  
> -	if (!IS_ERR(page)) {
> -		de = ufs_next_entry(dir->i_sb,
> -				    (struct ufs_dir_entry *)page_address(page));
> -		*p = page;
> -	}
> -	return de;
> +	if (!IS_ERR(de))
> +		return ufs_next_entry(dir->i_sb, de);
> +	else
> +		return NULL;
>  }
>  
>  /*
> @@ -273,11 +272,10 @@ struct ufs_dir_entry *ufs_find_entry(struct inode *dir, const struct qstr *qstr,
>  		start = 0;
>  	n = start;
>  	do {
> -		char *kaddr;
> -		page = ufs_get_page(dir, n);
> -		if (!IS_ERR(page)) {
> -			kaddr = page_address(page);
> -			de = (struct ufs_dir_entry *) kaddr;
> +		char *kaddr = ufs_get_page(dir, n, &page);
> +
> +		if (!IS_ERR(kaddr)) {
> +			de = (struct ufs_dir_entry *)kaddr;
>  			kaddr += ufs_last_byte(dir, n) - reclen;
>  			while ((char *) de <= kaddr) {
>  				if (ufs_match(sb, namelen, name, de))
> @@ -328,12 +326,10 @@ int ufs_add_link(struct dentry *dentry, struct inode *inode)
>  	for (n = 0; n <= npages; n++) {
>  		char *dir_end;
>  
> -		page = ufs_get_page(dir, n);
> -		err = PTR_ERR(page);
> -		if (IS_ERR(page))
> -			goto out;
> +		kaddr = ufs_get_page(dir, n, &page);
> +		if (IS_ERR(kaddr))
> +			return PTR_ERR(kaddr);
>  		lock_page(page);
> -		kaddr = page_address(page);
>  		dir_end = kaddr + ufs_last_byte(dir, n);
>  		de = (struct ufs_dir_entry *)kaddr;
>  		kaddr += PAGE_SIZE - reclen;
> @@ -395,7 +391,6 @@ int ufs_add_link(struct dentry *dentry, struct inode *inode)
>  	/* OFFSET_CACHE */
>  out_put:
>  	ufs_put_page(page);
> -out:
>  	return err;
>  out_unlock:
>  	unlock_page(page);
> @@ -429,6 +424,7 @@ ufs_readdir(struct file *file, struct dir_context *ctx)
>  	unsigned chunk_mask = ~(UFS_SB(sb)->s_uspi->s_dirblksize - 1);
>  	bool need_revalidate = !inode_eq_iversion(inode, file->f_version);
>  	unsigned flags = UFS_SB(sb)->s_flags;
> +	struct page *page;

NIT: Does page now leave the scope of the for loop?

>  
>  	UFSD("BEGIN\n");
>  
> @@ -439,16 +435,14 @@ ufs_readdir(struct file *file, struct dir_context *ctx)
>  		char *kaddr, *limit;
>  		struct ufs_dir_entry *de;

Couldn't that be declared here?

Regardless I don't think this is broken.

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

>  
> -		struct page *page = ufs_get_page(inode, n);
> -
> -		if (IS_ERR(page)) {
> +		kaddr = ufs_get_page(inode, n, &page);
> +		if (IS_ERR(kaddr)) {
>  			ufs_error(sb, __func__,
>  				  "bad page in #%lu",
>  				  inode->i_ino);
>  			ctx->pos += PAGE_SIZE - offset;
>  			return -EIO;
>  		}
> -		kaddr = page_address(page);
>  		if (unlikely(need_revalidate)) {
>  			if (offset) {
>  				offset = ufs_validate_entry(sb, kaddr, offset, chunk_mask);
> @@ -595,12 +589,11 @@ int ufs_empty_dir(struct inode * inode)
>  	for (i = 0; i < npages; i++) {
>  		char *kaddr;
>  		struct ufs_dir_entry *de;
> -		page = ufs_get_page(inode, i);
>  
> -		if (IS_ERR(page))
> +		kaddr = ufs_get_page(inode, i, &page);
> +		if (IS_ERR(kaddr))
>  			continue;
>  
> -		kaddr = page_address(page);
>  		de = (struct ufs_dir_entry *)kaddr;
>  		kaddr += ufs_last_byte(inode, i) - UFS_DIR_REC_LEN(1);
>  
> -- 
> 2.39.0
>
Fabio M. De Francesco Dec. 22, 2022, 2:43 p.m. UTC | #2 
On giovedì 22 dicembre 2022 06:13:43 CET Ira Weiny wrote:
> On Wed, Dec 21, 2022 at 06:28:01PM +0100, Fabio M. De Francesco wrote:
> > Change the signature of ufs_get_page() in order to prepare this function
> > to the conversion to the use of kmap_local_page(). Change also those call
> > sites which are required to conform its invocations to the new
> > signature.
> > 
> > Cc: Ira Weiny <ira.weiny@intel.com>
> > Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > ---
> > 
> >  fs/ufs/dir.c | 49 +++++++++++++++++++++----------------------------
> >  1 file changed, 21 insertions(+), 28 deletions(-)
> > 
> > diff --git a/fs/ufs/dir.c b/fs/ufs/dir.c
> > index 69f78583c9c1..9fa86614d2d1 100644
> > --- a/fs/ufs/dir.c
> > +++ b/fs/ufs/dir.c
> > @@ -185,7 +185,7 @@ static bool ufs_check_page(struct page *page)
> > 
> >  	return false;
> >  
> >  }
> > 
> > -static struct page *ufs_get_page(struct inode *dir, unsigned long n)
> > +static void *ufs_get_page(struct inode *dir, unsigned long n, struct page
> > **p)> 
> >  {
> >  
> >  	struct address_space *mapping = dir->i_mapping;
> >  	struct page *page = read_mapping_page(mapping, n, NULL);
> > 
> > @@ -195,8 +195,10 @@ static struct page *ufs_get_page(struct inode *dir,
> > unsigned long n)> 
> >  			if (!ufs_check_page(page))
> >  			
> >  				goto fail;
> >  		
> >  		}
> > 
> > +		*p = page;
> > +		return page_address(page);
> > 
> >  	}
> > 
> > -	return page;
> > +	return ERR_CAST(page);
> > 
> >  fail:
> >  	ufs_put_page(page);
> > 
> > @@ -227,15 +229,12 @@ ufs_next_entry(struct super_block *sb, struct
> > ufs_dir_entry *p)> 
> >  struct ufs_dir_entry *ufs_dotdot(struct inode *dir, struct page **p)
> >  {
> > 
> > -	struct page *page = ufs_get_page(dir, 0);
> > -	struct ufs_dir_entry *de = NULL;
> > +	struct ufs_dir_entry *de = ufs_get_page(dir, 0, p);
> 
> I don't know why but ufs_get_page() returning an address read really odd to
> me. But rolling around my head alternative names nothing seems better than
> this.

ufs_get_kaddr()?

> > -	if (!IS_ERR(page)) {
> > -		de = ufs_next_entry(dir->i_sb,
> > -				    (struct ufs_dir_entry 
*)page_address(page));
> > -		*p = page;
> > -	}
> > -	return de;
> > +	if (!IS_ERR(de))
> > +		return ufs_next_entry(dir->i_sb, de);
> > +	else
> > +		return NULL;
> > 
> >  }
> >  
> >  /*
> > 
> > @@ -273,11 +272,10 @@ struct ufs_dir_entry *ufs_find_entry(struct inode
> > *dir, const struct qstr *qstr,> 
> >  		start = 0;
> >  	
> >  	n = start;
> >  	do {
> > 
> > -		char *kaddr;
> > -		page = ufs_get_page(dir, n);
> > -		if (!IS_ERR(page)) {
> > -			kaddr = page_address(page);
> > -			de = (struct ufs_dir_entry *) kaddr;
> > +		char *kaddr = ufs_get_page(dir, n, &page);
> > +
> > +		if (!IS_ERR(kaddr)) {
> > +			de = (struct ufs_dir_entry *)kaddr;
> > 
> >  			kaddr += ufs_last_byte(dir, n) - reclen;
> >  			while ((char *) de <= kaddr) {
> >  			
> >  				if (ufs_match(sb, namelen, name, de))
> > 
> > @@ -328,12 +326,10 @@ int ufs_add_link(struct dentry *dentry, struct inode
> > *inode)> 
> >  	for (n = 0; n <= npages; n++) {
> >  	
> >  		char *dir_end;
> > 
> > -		page = ufs_get_page(dir, n);
> > -		err = PTR_ERR(page);
> > -		if (IS_ERR(page))
> > -			goto out;
> > +		kaddr = ufs_get_page(dir, n, &page);
> > +		if (IS_ERR(kaddr))
> > +			return PTR_ERR(kaddr);
> > 
> >  		lock_page(page);
> > 
> > -		kaddr = page_address(page);
> > 
> >  		dir_end = kaddr + ufs_last_byte(dir, n);
> >  		de = (struct ufs_dir_entry *)kaddr;
> >  		kaddr += PAGE_SIZE - reclen;
> > 
> > @@ -395,7 +391,6 @@ int ufs_add_link(struct dentry *dentry, struct inode
> > *inode)> 
> >  	/* OFFSET_CACHE */
> >  
> >  out_put:
> >  	ufs_put_page(page);
> > 
> > -out:
> >  	return err;
> >  
> >  out_unlock:
> >  	unlock_page(page);
> > 
> > @@ -429,6 +424,7 @@ ufs_readdir(struct file *file, struct dir_context 
*ctx)
> > 
> >  	unsigned chunk_mask = ~(UFS_SB(sb)->s_uspi->s_dirblksize - 1);
> >  	bool need_revalidate = !inode_eq_iversion(inode, file->f_version);
> >  	unsigned flags = UFS_SB(sb)->s_flags;
> > 
> > +	struct page *page;
> 
> NIT: Does page now leave the scope of the for loop?
> 

Strange...
I can't say why I did so.

> >  	UFSD("BEGIN\n");
> > 
> > @@ -439,16 +435,14 @@ ufs_readdir(struct file *file, struct dir_context
> > *ctx)
> > 
> >  		char *kaddr, *limit;
> >  		struct ufs_dir_entry *de;
> 
> Couldn't that be declared here?

Yes, it could :-)

> Regardless I don't think this is broken.

Since I have to submit a new version of this series, there's no problem moving 
the declaration of "page" back into the loop.

> Reviewed-by: Ira Weiny <ira.weiny@intel.com>

Thanks,

Fabio
>
> > -		struct page *page = ufs_get_page(inode, n);
> > -
> > -		if (IS_ERR(page)) {
> > +		kaddr = ufs_get_page(inode, n, &page);
> > +		if (IS_ERR(kaddr)) {
> > 
> >  			ufs_error(sb, __func__,
> >  			
> >  				  "bad page in #%lu",
> >  				  inode->i_ino);
> >  			
> >  			ctx->pos += PAGE_SIZE - offset;
> >  			return -EIO;
> >  		
> >  		}
> > 
> > -		kaddr = page_address(page);
> > 
> >  		if (unlikely(need_revalidate)) {
> >  		
> >  			if (offset) {
> >  			
> >  				offset = ufs_validate_entry(sb, kaddr, 
offset, chunk_mask);
> > 
> > @@ -595,12 +589,11 @@ int ufs_empty_dir(struct inode * inode)
> > 
> >  	for (i = 0; i < npages; i++) {
> >  	
> >  		char *kaddr;
> >  		struct ufs_dir_entry *de;
> > 
> > -		page = ufs_get_page(inode, i);
> > 
> > -		if (IS_ERR(page))
> > +		kaddr = ufs_get_page(inode, i, &page);
> > +		if (IS_ERR(kaddr))
> > 
> >  			continue;
> > 
> > -		kaddr = page_address(page);
> > 
> >  		de = (struct ufs_dir_entry *)kaddr;
> >  		kaddr += ufs_last_byte(inode, i) - UFS_DIR_REC_LEN(1);
> > 
> > --
> > 2.39.0
diff mbox series

Patch

diff --git a/fs/ufs/dir.c b/fs/ufs/dir.c
index 69f78583c9c1..9fa86614d2d1 100644
--- a/fs/ufs/dir.c
+++ b/fs/ufs/dir.c
@@ -185,7 +185,7 @@  static bool ufs_check_page(struct page *page)
 	return false;
 }
 
-static struct page *ufs_get_page(struct inode *dir, unsigned long n)
+static void *ufs_get_page(struct inode *dir, unsigned long n, struct page **p)
 {
 	struct address_space *mapping = dir->i_mapping;
 	struct page *page = read_mapping_page(mapping, n, NULL);
@@ -195,8 +195,10 @@  static struct page *ufs_get_page(struct inode *dir, unsigned long n)
 			if (!ufs_check_page(page))
 				goto fail;
 		}
+		*p = page;
+		return page_address(page);
 	}
-	return page;
+	return ERR_CAST(page);
 
 fail:
 	ufs_put_page(page);
@@ -227,15 +229,12 @@  ufs_next_entry(struct super_block *sb, struct ufs_dir_entry *p)
 
 struct ufs_dir_entry *ufs_dotdot(struct inode *dir, struct page **p)
 {
-	struct page *page = ufs_get_page(dir, 0);
-	struct ufs_dir_entry *de = NULL;
+	struct ufs_dir_entry *de = ufs_get_page(dir, 0, p);
 
-	if (!IS_ERR(page)) {
-		de = ufs_next_entry(dir->i_sb,
-				    (struct ufs_dir_entry *)page_address(page));
-		*p = page;
-	}
-	return de;
+	if (!IS_ERR(de))
+		return ufs_next_entry(dir->i_sb, de);
+	else
+		return NULL;
 }
 
 /*
@@ -273,11 +272,10 @@  struct ufs_dir_entry *ufs_find_entry(struct inode *dir, const struct qstr *qstr,
 		start = 0;
 	n = start;
 	do {
-		char *kaddr;
-		page = ufs_get_page(dir, n);
-		if (!IS_ERR(page)) {
-			kaddr = page_address(page);
-			de = (struct ufs_dir_entry *) kaddr;
+		char *kaddr = ufs_get_page(dir, n, &page);
+
+		if (!IS_ERR(kaddr)) {
+			de = (struct ufs_dir_entry *)kaddr;
 			kaddr += ufs_last_byte(dir, n) - reclen;
 			while ((char *) de <= kaddr) {
 				if (ufs_match(sb, namelen, name, de))
@@ -328,12 +326,10 @@  int ufs_add_link(struct dentry *dentry, struct inode *inode)
 	for (n = 0; n <= npages; n++) {
 		char *dir_end;
 
-		page = ufs_get_page(dir, n);
-		err = PTR_ERR(page);
-		if (IS_ERR(page))
-			goto out;
+		kaddr = ufs_get_page(dir, n, &page);
+		if (IS_ERR(kaddr))
+			return PTR_ERR(kaddr);
 		lock_page(page);
-		kaddr = page_address(page);
 		dir_end = kaddr + ufs_last_byte(dir, n);
 		de = (struct ufs_dir_entry *)kaddr;
 		kaddr += PAGE_SIZE - reclen;
@@ -395,7 +391,6 @@  int ufs_add_link(struct dentry *dentry, struct inode *inode)
 	/* OFFSET_CACHE */
 out_put:
 	ufs_put_page(page);
-out:
 	return err;
 out_unlock:
 	unlock_page(page);
@@ -429,6 +424,7 @@  ufs_readdir(struct file *file, struct dir_context *ctx)
 	unsigned chunk_mask = ~(UFS_SB(sb)->s_uspi->s_dirblksize - 1);
 	bool need_revalidate = !inode_eq_iversion(inode, file->f_version);
 	unsigned flags = UFS_SB(sb)->s_flags;
+	struct page *page;
 
 	UFSD("BEGIN\n");
 
@@ -439,16 +435,14 @@  ufs_readdir(struct file *file, struct dir_context *ctx)
 		char *kaddr, *limit;
 		struct ufs_dir_entry *de;
 
-		struct page *page = ufs_get_page(inode, n);
-
-		if (IS_ERR(page)) {
+		kaddr = ufs_get_page(inode, n, &page);
+		if (IS_ERR(kaddr)) {
 			ufs_error(sb, __func__,
 				  "bad page in #%lu",
 				  inode->i_ino);
 			ctx->pos += PAGE_SIZE - offset;
 			return -EIO;
 		}
-		kaddr = page_address(page);
 		if (unlikely(need_revalidate)) {
 			if (offset) {
 				offset = ufs_validate_entry(sb, kaddr, offset, chunk_mask);
@@ -595,12 +589,11 @@  int ufs_empty_dir(struct inode * inode)
 	for (i = 0; i < npages; i++) {
 		char *kaddr;
 		struct ufs_dir_entry *de;
-		page = ufs_get_page(inode, i);
 
-		if (IS_ERR(page))
+		kaddr = ufs_get_page(inode, i, &page);
+		if (IS_ERR(kaddr))
 			continue;
 
-		kaddr = page_address(page);
 		de = (struct ufs_dir_entry *)kaddr;
 		kaddr += ufs_last_byte(inode, i) - UFS_DIR_REC_LEN(1);