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 to the new signature.
Cc: Ira Weiny <[email protected]>
Suggested-by: Al Viro <[email protected]>
Signed-off-by: Fabio M. De Francesco <[email protected]>
---
Delete an unnecessary assignment from v1 (thanks to Dan).
Reported-by: Dan Carpenter <[email protected]>
fs/sysv/dir.c | 51 ++++++++++++++++++++++++---------------------------
1 file changed, 24 insertions(+), 27 deletions(-)
diff --git a/fs/sysv/dir.c b/fs/sysv/dir.c
index 685379bc9d64..f953e6b9251e 100644
--- a/fs/sysv/dir.c
+++ b/fs/sysv/dir.c
@@ -52,13 +52,15 @@ static int dir_commit_chunk(struct page *page, loff_t pos, unsigned len)
return err;
}
-static struct page * dir_get_page(struct inode *dir, unsigned long n)
+static void *dir_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);
- if (!IS_ERR(page))
- kmap(page);
- return page;
+ if (IS_ERR(page))
+ return ERR_CAST(page);
+ kmap(page);
+ *p = page;
+ return page_address(page);
}
static int sysv_readdir(struct file *file, struct dir_context *ctx)
@@ -80,11 +82,11 @@ static int sysv_readdir(struct file *file, struct dir_context *ctx)
for ( ; n < npages; n++, offset = 0) {
char *kaddr, *limit;
struct sysv_dir_entry *de;
- struct page *page = dir_get_page(inode, n);
+ struct page *page;
- if (IS_ERR(page))
+ kaddr = dir_get_page(inode, n, &page);
+ if (IS_ERR(kaddr))
continue;
- kaddr = (char *)page_address(page);
de = (struct sysv_dir_entry *)(kaddr+offset);
limit = kaddr + PAGE_SIZE - SYSV_DIRSIZE;
for ( ;(char*)de <= limit; de++, ctx->pos += sizeof(*de)) {
@@ -142,11 +144,10 @@ struct sysv_dir_entry *sysv_find_entry(struct dentry *dentry, struct page **res_
n = start;
do {
- char *kaddr;
- page = dir_get_page(dir, n);
- if (!IS_ERR(page)) {
- kaddr = (char*)page_address(page);
- de = (struct sysv_dir_entry *) kaddr;
+ char *kaddr = dir_get_page(dir, n, &page);
+
+ if (!IS_ERR(kaddr)) {
+ de = (struct sysv_dir_entry *)kaddr;
kaddr += PAGE_SIZE - SYSV_DIRSIZE;
for ( ; (char *) de <= kaddr ; de++) {
if (!de->inode)
@@ -185,11 +186,9 @@ int sysv_add_link(struct dentry *dentry, struct inode *inode)
/* We take care of directory expansion in the same loop */
for (n = 0; n <= npages; n++) {
- page = dir_get_page(dir, n);
- err = PTR_ERR(page);
- if (IS_ERR(page))
- goto out;
- kaddr = (char*)page_address(page);
+ kaddr = dir_get_page(dir, n, &page);
+ if (IS_ERR(kaddr))
+ return PTR_ERR(kaddr);
de = (struct sysv_dir_entry *)kaddr;
kaddr += PAGE_SIZE - SYSV_DIRSIZE;
while ((char *)de <= kaddr) {
@@ -219,7 +218,6 @@ int sysv_add_link(struct dentry *dentry, struct inode *inode)
mark_inode_dirty(dir);
out_page:
dir_put_page(page);
-out:
return err;
out_unlock:
unlock_page(page);
@@ -288,12 +286,11 @@ int sysv_empty_dir(struct inode * inode)
for (i = 0; i < npages; i++) {
char *kaddr;
struct sysv_dir_entry * de;
- page = dir_get_page(inode, i);
- if (IS_ERR(page))
+ kaddr = dir_get_page(inode, i, &page);
+ if (IS_ERR(kaddr))
continue;
- kaddr = (char *)page_address(page);
de = (struct sysv_dir_entry *)kaddr;
kaddr += PAGE_SIZE-SYSV_DIRSIZE;
@@ -339,16 +336,16 @@ void sysv_set_link(struct sysv_dir_entry *de, struct page *page,
mark_inode_dirty(dir);
}
-struct sysv_dir_entry * sysv_dotdot (struct inode *dir, struct page **p)
+struct sysv_dir_entry *sysv_dotdot(struct inode *dir, struct page **p)
{
- struct page *page = dir_get_page(dir, 0);
- struct sysv_dir_entry *de = NULL;
+ struct page *page = NULL;
+ struct sysv_dir_entry *de = dir_get_page(dir, 0, &page);
- if (!IS_ERR(page)) {
- de = (struct sysv_dir_entry*) page_address(page) + 1;
+ if (!IS_ERR(de)) {
*p = page;
+ return (struct sysv_dir_entry *)page_address(page) + 1;
}
- return de;
+ return NULL;
}
ino_t sysv_inode_by_name(struct dentry *dentry)
--
2.39.0
On Mon, Jan 09, 2023 at 06:06:37PM +0100, Fabio M. De Francesco wrote:
>
> -struct sysv_dir_entry * sysv_dotdot (struct inode *dir, struct page **p)
> +struct sysv_dir_entry *sysv_dotdot(struct inode *dir, struct page **p)
> {
> - struct page *page = dir_get_page(dir, 0);
> - struct sysv_dir_entry *de = NULL;
> + struct page *page = NULL;
> + struct sysv_dir_entry *de = dir_get_page(dir, 0, &page);
>
> - if (!IS_ERR(page)) {
> - de = (struct sysv_dir_entry*) page_address(page) + 1;
> + if (!IS_ERR(de)) {
> *p = page;
> + return (struct sysv_dir_entry *)page_address(page) + 1;
> }
> - return de;
> + return NULL;
> }
Would be better off with
struct sysv_dir_entry *de = dir_get_page(dir, 0, p);
if (!IS_ERR(de))
return de + 1; // ".." is the second directory entry
return NULL;
IMO...
On mercoled? 11 gennaio 2023 03:34:47 CET Al Viro wrote:
> On Mon, Jan 09, 2023 at 06:06:37PM +0100, Fabio M. De Francesco wrote:
> > -struct sysv_dir_entry * sysv_dotdot (struct inode *dir, struct page **p)
> > +struct sysv_dir_entry *sysv_dotdot(struct inode *dir, struct page **p)
> >
> > {
> >
> > - struct page *page = dir_get_page(dir, 0);
> > - struct sysv_dir_entry *de = NULL;
> > + struct page *page = NULL;
> > + struct sysv_dir_entry *de = dir_get_page(dir, 0, &page);
> >
> > - if (!IS_ERR(page)) {
> > - de = (struct sysv_dir_entry*) page_address(page) + 1;
> > + if (!IS_ERR(de)) {
> >
> > *p = page;
> >
> > + return (struct sysv_dir_entry *)page_address(page) + 1;
> >
> > }
> >
> > - return de;
> > + return NULL;
> >
> > }
>
> Would be better off with
>
> struct sysv_dir_entry *de = dir_get_page(dir, 0, p);
>
> if (!IS_ERR(de))
> return de + 1; // ".." is the second directory entry
> return NULL;
>
> IMO...
I totally agree with you...
1) This comment is a good way to explain why we return "de + 1".
2) "*p = page" is redundant, so it's not necessary because we assign the out
argument in dir_get_page() if and only if read_mapping_page() doesn't fail.
I will send v3 asap.
Thanks for your suggestions.
Fabio