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 | 58 ++++++++++++++++++++++------------------------------
1 file changed, 24 insertions(+), 34 deletions(-)
diff --git a/fs/ufs/dir.c b/fs/ufs/dir.c
index 69f78583c9c1..bb6b29ce1d3a 100644
--- a/fs/ufs/dir.c
+++ b/fs/ufs/dir.c
@@ -185,21 +185,21 @@ 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 **page)
{
struct address_space *mapping = dir->i_mapping;
- struct page *page = read_mapping_page(mapping, n, NULL);
- if (!IS_ERR(page)) {
- kmap(page);
- if (unlikely(!PageChecked(page))) {
- if (!ufs_check_page(page))
+ *page = read_mapping_page(mapping, n, NULL);
+ if (!IS_ERR(*page)) {
+ kmap(*page);
+ if (unlikely(!PageChecked(*page))) {
+ if (!ufs_check_page(*page))
goto fail;
}
}
return page;
fail:
- ufs_put_page(page);
+ ufs_put_page(*page);
return ERR_PTR(-EIO);
}
@@ -227,15 +227,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 +270,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 +324,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,8 +389,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);
goto out_put;
@@ -429,6 +421,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 +432,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 +586,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.38.1On Sun, Dec 11, 2022 at 10:31:10PM +0100, Fabio M. De Francesco wrote: > out_put: > ufs_put_page(page); > -out: > - return err; > out_unlock: > unlock_page(page); > goto out_put; Something strange has happened, all right - look at the situation after that patch. You've got out_put: ufs_put_page(page); out_unlock: unlock_page(page); goto out_put; Which is obviously bogus.
On domenica 11 dicembre 2022 23:42:26 CET Al Viro wrote:
> On Sun, Dec 11, 2022 at 10:31:10PM +0100, Fabio M. De Francesco wrote:
> > out_put:
> > ufs_put_page(page);
> >
> > -out:
> > - return err;
> >
> > out_unlock:
> > unlock_page(page);
> > goto out_put;
>
> Something strange has happened, all right - look at the situation
> after that patch. You've got
>
> out_put:
> ufs_put_page(page);
> out_unlock:
> unlock_page(page);
> goto out_put;
>
> Which is obviously bogus.
I finally could go back to this small series and while working to fix the
errors that yesterday you had found out I think I saw what happened...
Are you talking about ufs_add_link, right?
If so, you wrote what follows at point 14 of one of your emails:
-----
14) ufs_add_link() - similar adjustment to new calling conventions
for ufs_get_page(). Uses of page_addr: fed to ufs_put_page() (same as
in ufs_find_entry() kaddr is guaranteed to point into the same page and
thus can be used instead) and calculation of position in directory, same
as we'd seen in ufs_set_link(). The latter becomes page_offset(page) +
offset_in_page(de), killing page_addr off. BTW, we get
kaddr = ufs_get_page(dir, n, &page);
err = PTR_ERR(kaddr);
if (IS_ERR(kaddr))
goto out;
with out: being just 'return err;', which suggests
kaddr = ufs_get_page(dir, n, &page);
if (IS_ERR(kaddr))
return ERR_PTR(kaddr);
instead (and that was the only goto out; so the label can be removed).
The value stored in err in case !IS_ERR(kaddr) is (thankfully) never
used - would've been a bug otherwise. So this is an equivalent
transformation.
-----
Did you notice "so the label can be removed"?
I must have misinterpreted what you wrote there. Did I?
I removed the "out" label, according to what it seemed to me the correct way
to interpret your words.
However at that moment I didn't see the endless loop at the end of the
function. Then I "fixed" (sigh!) it in 3/3 by terminating that endless loop
with a "return 0".
However that was another mistake because after "got_it:" label we have "err =
ufs_commit_chunk(page, pos, rec_len);".
To summarize: I can delete _only_ the label and leave the "return err;" in the
block after the "out_put:" label.
Am I looking at it correctly now?
Thanks,
Fabio
On Sun, Dec 11, 2022 at 10:31:10PM +0100, Fabio M. De Francesco wrote:
> -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 **page)
> {
> struct address_space *mapping = dir->i_mapping;
> - struct page *page = read_mapping_page(mapping, n, NULL);
> - if (!IS_ERR(page)) {
> - kmap(page);
> - if (unlikely(!PageChecked(page))) {
> - if (!ufs_check_page(page))
> + *page = read_mapping_page(mapping, n, NULL);
> + if (!IS_ERR(*page)) {
> + kmap(*page);
> + if (unlikely(!PageChecked(*page))) {
> + if (!ufs_check_page(*page))
> goto fail;
> }
> }
> return page;
return page_address(page), surely?
>
> fail:
> - ufs_put_page(page);
> + ufs_put_page(*page);
> return ERR_PTR(-EIO);
> }
>
> @@ -227,15 +227,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);
... considering e.g. this. The caller expects the pointer to beginning of
page, not pointer to struct page itself. Other callers are also like that...
On domenica 11 dicembre 2022 23:29:31 CET Al Viro wrote:
> On Sun, Dec 11, 2022 at 10:31:10PM +0100, Fabio M. De Francesco wrote:
> > -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
> > **page)>
> > {
> >
> > struct address_space *mapping = dir->i_mapping;
> >
> > - struct page *page = read_mapping_page(mapping, n, NULL);
> > - if (!IS_ERR(page)) {
> > - kmap(page);
> > - if (unlikely(!PageChecked(page))) {
> > - if (!ufs_check_page(page))
> > + *page = read_mapping_page(mapping, n, NULL);
> > + if (!IS_ERR(*page)) {
> > + kmap(*page);
> > + if (unlikely(!PageChecked(*page))) {
> > + if (!ufs_check_page(*page))
> >
> > goto fail;
> >
> > }
> >
> > }
> > return page;
>
> return page_address(page), surely?
>
Yes, I'm sorry for these kinds of silly mistakes because while I was expecting
to err on much more difficult topics I overlooked what I know pretty well :-(
Shouldn't it be "return page_address(*page)" because of "page" being a pointer
to pointer to "struct page"? Am I overlooking something?
However, the greater mistake was about doing the decomposition into three
patches starting from the old single thing. I think that I had to start from
scratch. I made the process the other way round.
>
> > fail:
> > - ufs_put_page(page);
> > + ufs_put_page(*page);
> >
> > return ERR_PTR(-EIO);
> >
> > }
> >
> > @@ -227,15 +227,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);
>
> ... considering e.g. this. The caller expects the pointer to beginning of
> page, not pointer to struct page itself. Other callers are also like
that...
>
I'll send next version within tomorrow (before or after work time) because
here it is 00.40 CET.
Thank you very much for your immediate reply.
Fabio
© 2016 - 2025 Red Hat, Inc.