| Message ID | 20221211213111.30085-3-fmdefrancesco@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fs/ufs: replace kmap() with kmap_local_page() | expand |
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 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: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
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
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);
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(-)