| Message ID | 20240417150416.752929-2-willy@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Convert UDF to folios | expand |
On Wed 17-04-24 16:04:07, Matthew Wilcox (Oracle) wrote: > Remove the conversion to struct page and use folio APIs throughout. > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> Looks good. I've just noticed this removes the SetPageError(). Grepping for a while we seem to set/clear that in quite some places in the filesystems but nobody is reading it these days (to be fair jfs has one test and btrfs also one)? And similarly with folio_test_error... I have a recollection this was actually used in the past but maybe you've removed it as part of folio overhaul? Anyway, either something should start using the error bit or we can drop the dead code and free up a page flags bit. Yay. Honza > --- > fs/udf/symlink.c | 22 ++++++---------------- > 1 file changed, 6 insertions(+), 16 deletions(-) > > diff --git a/fs/udf/symlink.c b/fs/udf/symlink.c > index f7eaf7b14594..0105e7e2ba3d 100644 > --- a/fs/udf/symlink.c > +++ b/fs/udf/symlink.c > @@ -99,18 +99,17 @@ static int udf_pc_to_char(struct super_block *sb, unsigned char *from, > > static int udf_symlink_filler(struct file *file, struct folio *folio) > { > - struct page *page = &folio->page; > - struct inode *inode = page->mapping->host; > + struct inode *inode = folio->mapping->host; > struct buffer_head *bh = NULL; > unsigned char *symlink; > int err = 0; > - unsigned char *p = page_address(page); > + unsigned char *p = folio_address(folio); > struct udf_inode_info *iinfo = UDF_I(inode); > > /* We don't support symlinks longer than one block */ > if (inode->i_size > inode->i_sb->s_blocksize) { > err = -ENAMETOOLONG; > - goto out_unlock; > + goto out; > } > > if (iinfo->i_alloc_type == ICBTAG_FLAG_AD_IN_ICB) { > @@ -120,24 +119,15 @@ static int udf_symlink_filler(struct file *file, struct folio *folio) > if (!bh) { > if (!err) > err = -EFSCORRUPTED; > - goto out_err; > + goto out; > } > symlink = bh->b_data; > } > > err = udf_pc_to_char(inode->i_sb, symlink, inode->i_size, p, PAGE_SIZE); > brelse(bh); > - if (err) > - goto out_err; > - > - SetPageUptodate(page); > - unlock_page(page); > - return 0; > - > -out_err: > - SetPageError(page); > -out_unlock: > - unlock_page(page); > +out: > + folio_end_read(folio, err == 0); > return err; > } > > -- > 2.43.0 >
On Thu, Apr 18, 2024 at 12:37:34PM +0200, Jan Kara wrote: > On Wed 17-04-24 16:04:07, Matthew Wilcox (Oracle) wrote: > > Remove the conversion to struct page and use folio APIs throughout. > > > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > > Looks good. I've just noticed this removes the SetPageError(). Grepping for > a while we seem to set/clear that in quite some places in the filesystems > but nobody is reading it these days (to be fair jfs has one test and btrfs > also one)? And similarly with folio_test_error... I have a recollection > this was actually used in the past but maybe you've removed it as part of > folio overhaul? Anyway, either something should start using the error bit > or we can drop the dead code and free up a page flags bit. Yay. Right, the VFS never checks PageError nor folio_test_error. It's purely a filesystem-internal-use flag at this point. I think buffer.c used to test it, but only locally, so I turned it into a local bool. So all the places in filesystems which set/clear it can be removed ... except for the filesystems which check it. I'd love to reclaim that flag, I just need to figure out how to remove the few remaining places that check it. The btrfs usage is awful because PageError was _supposed_ to be used for read errors, but they're using it for writeback errors. And they're using that flag on the bdev's page cache, not even their own page cache. It's also buggy for machines with PAGE_SIZE > 4kB; it's just that writeback errors are rare, so they get away with it. I had a go at fixing it once, but failed. JFS is more straightforward; I think I can use a bit in struct metapage as a replacement read error flag. I should probably have another go at fixing these two and then I can reclaim PG_error.
diff --git a/fs/udf/symlink.c b/fs/udf/symlink.c index f7eaf7b14594..0105e7e2ba3d 100644 --- a/fs/udf/symlink.c +++ b/fs/udf/symlink.c @@ -99,18 +99,17 @@ static int udf_pc_to_char(struct super_block *sb, unsigned char *from, static int udf_symlink_filler(struct file *file, struct folio *folio) { - struct page *page = &folio->page; - struct inode *inode = page->mapping->host; + struct inode *inode = folio->mapping->host; struct buffer_head *bh = NULL; unsigned char *symlink; int err = 0; - unsigned char *p = page_address(page); + unsigned char *p = folio_address(folio); struct udf_inode_info *iinfo = UDF_I(inode); /* We don't support symlinks longer than one block */ if (inode->i_size > inode->i_sb->s_blocksize) { err = -ENAMETOOLONG; - goto out_unlock; + goto out; } if (iinfo->i_alloc_type == ICBTAG_FLAG_AD_IN_ICB) { @@ -120,24 +119,15 @@ static int udf_symlink_filler(struct file *file, struct folio *folio) if (!bh) { if (!err) err = -EFSCORRUPTED; - goto out_err; + goto out; } symlink = bh->b_data; } err = udf_pc_to_char(inode->i_sb, symlink, inode->i_size, p, PAGE_SIZE); brelse(bh); - if (err) - goto out_err; - - SetPageUptodate(page); - unlock_page(page); - return 0; - -out_err: - SetPageError(page); -out_unlock: - unlock_page(page); +out: + folio_end_read(folio, err == 0); return err; }
Remove the conversion to struct page and use folio APIs throughout. Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> --- fs/udf/symlink.c | 22 ++++++---------------- 1 file changed, 6 insertions(+), 16 deletions(-)