From: Yu Kuai <redacted>
Changes in v3:
- remove bdev_associated_mapping() and patch 12 from v1;
- add kerneldoc comments for new bdev apis;
- rename __bdev_get_folio() to bdev_get_folio;
- fix a problem in erofs that erofs_init_metabuf() is not always
called.
- add reviewed-by tag for patch 15-17;
Changes in v2:
- remove some bdev apis that is not necessary;
- pass in offset for bdev_read_folio() and __bdev_get_folio();
- remove bdev_gfp_constraint() and add a new helper in fs/buffer.c to
prevent access bd_indoe() directly from mapping_gfp_constraint() in
ext4.(patch 15, 16);
- remove block_device_ejected() from ext4.
Patch 1 add some bdev apis, then follow up patches will use these apis
to avoid access bd_inode directly, and hopefully the field bd_inode can
be removed eventually(after figure out a way for fs/buffer.c).
Yu Kuai (17):
block: add some bdev apis
xen/blkback: use bdev api in xen_update_blkif_status()
bcache: use bdev api in read_super()
mtd: block2mtd: use bdev apis
s390/dasd: use bdev api in dasd_format()
scsicam: use bdev api in scsi_bios_ptable()
bcachefs: remove dead function bdev_sectors()
bio: export bio_add_folio_nofail()
btrfs: use bdev apis
cramfs: use bdev apis in cramfs_blkdev_read()
erofs: use bdev api
nilfs2: use bdev api in nilfs_attach_log_writer()
jbd2: use bdev apis
buffer: add a new helper to read sb block
ext4: use new helper to read sb block
ext4: remove block_device_ejected()
ext4: use bdev apis
block/bdev.c | 148 +++++++++++++++++++++++++++++
block/bio.c | 1 +
block/blk.h | 2 -
drivers/block/xen-blkback/xenbus.c | 3 +-
drivers/md/bcache/super.c | 11 +--
drivers/mtd/devices/block2mtd.c | 81 +++++++---------
drivers/s390/block/dasd_ioctl.c | 5 +-
drivers/scsi/scsicam.c | 4 +-
fs/bcachefs/util.h | 5 -
fs/btrfs/disk-io.c | 71 +++++++-------
fs/btrfs/volumes.c | 17 ++--
fs/btrfs/zoned.c | 15 +--
fs/buffer.c | 68 +++++++++----
fs/cramfs/inode.c | 36 +++----
fs/erofs/data.c | 18 ++--
fs/erofs/internal.h | 2 +
fs/ext4/dir.c | 6 +-
fs/ext4/ext4.h | 13 ---
fs/ext4/ext4_jbd2.c | 6 +-
fs/ext4/inode.c | 8 +-
fs/ext4/super.c | 66 +++----------
fs/ext4/symlink.c | 2 +-
fs/jbd2/journal.c | 3 +-
fs/jbd2/recovery.c | 6 +-
fs/nilfs2/segment.c | 2 +-
include/linux/blkdev.h | 17 ++++
include/linux/buffer_head.h | 18 +++-
27 files changed, 377 insertions(+), 257 deletions(-)
--
2.39.2
From: Yu Kuai <redacted>
Those apis will be used for other modules, so that bd_inode won't be
accessed directly from other modules.
Signed-off-by: Yu Kuai <redacted>
---
block/bdev.c | 148 +++++++++++++++++++++++++++++++++++++++++
block/blk.h | 2 -
include/linux/blkdev.h | 17 +++++
3 files changed, 165 insertions(+), 2 deletions(-)
From: Yu Kuai <redacted>
On the one hand covert to use folio while reading bdev inode, on the
other hand prevent to access bd_inode directly.
Signed-off-by: Yu Kuai <redacted>
---
drivers/md/bcache/super.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
From: Yu Kuai <redacted>
On the one hand covert to use folio while reading bdev inode, on the
other hand prevent to access bd_inode directly.
Signed-off-by: Yu Kuai <redacted>
---
drivers/mtd/devices/block2mtd.c | 81 +++++++++++++++------------------
1 file changed, 36 insertions(+), 45 deletions(-)
@@ -46,40 +46,34 @@ struct block2mtd_dev {/* Static info about the MTD, used in cleanup_module */staticLIST_HEAD(blkmtd_device_list);--staticstructpage*page_read(structaddress_space*mapping,pgoff_tindex)-{-returnread_mapping_page(mapping,index,NULL);-}-/* erase a specified part of the device */staticint_block2mtd_erase(structblock2mtd_dev*dev,loff_tto,size_tlen){-structaddress_space*mapping=-dev->bdev_handle->bdev->bd_inode->i_mapping;-structpage*page;+structblock_device*bdev=dev->bdev_handle->bdev;+structfolio*folio;pgoff_tindex=to>>PAGE_SHIFT;// page indexintpages=len>>PAGE_SHIFT;u_long*p;u_long*max;while(pages){-page=page_read(mapping,index);-if(IS_ERR(page))-returnPTR_ERR(page);+folio=bdev_read_folio(bdev,index<<PAGE_SHIFT);+if(IS_ERR(folio))+returnPTR_ERR(folio);-max=page_address(page)+PAGE_SIZE;-for(p=page_address(page);p<max;p++)+max=folio_address(folio)+folio_size(folio);+for(p=folio_address(folio);p<max;p++)if(*p!=-1UL){-lock_page(page);-memset(page_address(page),0xff,PAGE_SIZE);-set_page_dirty(page);-unlock_page(page);-balance_dirty_pages_ratelimited(mapping);+folio_lock(folio);+memset(folio_address(folio),0xff,+folio_size(folio));+folio_mark_dirty(folio);+folio_unlock(folio);+bdev_balance_dirty_pages_ratelimited(bdev);break;}-put_page(page);+folio_put(folio);pages--;index++;}
@@ -295,7 +286,7 @@ static struct block2mtd_dev *add_device(char *devname, int erase_size,gotoerr_free_block2mtd;}-if((long)bdev->bd_inode->i_size%erase_size){+if(bdev_nr_bytes(bdev)%erase_size){pr_err("erasesize must be a divisor of device size\n");gotoerr_free_block2mtd;}
From: Yu Kuai <redacted>
Currently btrfs is using __bio_add_page() in write_dev_supers(). In order
to convert to use folio for bdev in btrfs, export bio_add_folio_nofail()
so that it can replace __bio_add_page().
Signed-off-by: Yu Kuai <redacted>
---
block/bio.c | 1 +
1 file changed, 1 insertion(+)
From: Yu Kuai <redacted>
On the one hand covert to use folio while reading bdev inode, on the
other hand prevent to access bd_inode directly.
Signed-off-by: Yu Kuai <redacted>
---
fs/btrfs/disk-io.c | 71 +++++++++++++++++++++-------------------------
fs/btrfs/volumes.c | 17 ++++++-----
fs/btrfs/zoned.c | 15 +++++-----
3 files changed, 48 insertions(+), 55 deletions(-)
@@ -3620,28 +3620,24 @@ ALLOW_ERROR_INJECTION(open_ctree, ERRNO);staticvoidbtrfs_end_super_write(structbio*bio){structbtrfs_device*device=bio->bi_private;-structbio_vec*bvec;-structbvec_iter_alliter_all;-structpage*page;--bio_for_each_segment_all(bvec,bio,iter_all){-page=bvec->bv_page;+structfolio_iterfi;+bio_for_each_folio_all(fi,bio){if(bio->bi_status){btrfs_warn_rl_in_rcu(device->fs_info,"lost page write due to IO error on %s (%d)",btrfs_dev_name(device),blk_status_to_errno(bio->bi_status));-ClearPageUptodate(page);-SetPageError(page);+folio_clear_uptodate(fi.folio);+folio_set_error(fi.folio);btrfs_dev_stat_inc_and_print(device,BTRFS_DEV_STAT_WRITE_ERRS);}else{-SetPageUptodate(page);+folio_mark_uptodate(fi.folio);}-put_page(page);-unlock_page(page);+folio_put(fi.folio);+folio_unlock(fi.folio);}bio_put(bio);
@@ -3740,7 +3737,6 @@ static int write_dev_supers(struct btrfs_device *device,structbtrfs_super_block*sb,intmax_mirrors){structbtrfs_fs_info*fs_info=device->fs_info;-structaddress_space*mapping=device->bdev->bd_inode->i_mapping;SHASH_DESC_ON_STACK(shash,fs_info->csum_shash);inti;interrors=0;
@@ -3753,7 +3749,7 @@ static int write_dev_supers(struct btrfs_device *device,shash->tfm=fs_info->csum_shash;for(i=0;i<max_mirrors;i++){-structpage*page;+structfolio*folio;structbio*bio;structbtrfs_super_block*disk_super;
@@ -3778,9 +3774,10 @@ static int write_dev_supers(struct btrfs_device *device,BTRFS_SUPER_INFO_SIZE-BTRFS_CSUM_SIZE,sb->csum);-page=find_or_create_page(mapping,bytenr>>PAGE_SHIFT,-GFP_NOFS);-if(!page){+folio=bdev_get_folio(device->bdev,bytenr,+FGP_LOCK|FGP_ACCESSED|FGP_CREAT,+GFP_NOFS);+if(IS_ERR(folio)){btrfs_err(device->fs_info,"couldn't get super block page for bytenr %llu",bytenr);
@@ -3789,9 +3786,9 @@ static int write_dev_supers(struct btrfs_device *device,}/* Bump the refcount for wait_dev_supers() */-get_page(page);+folio_get(folio);-disk_super=page_address(page);+disk_super=folio_address(folio);memcpy(disk_super,sb,BTRFS_SUPER_INFO_SIZE);/*
@@ -3805,8 +3802,8 @@ static int write_dev_supers(struct btrfs_device *device,bio->bi_iter.bi_sector=bytenr>>SECTOR_SHIFT;bio->bi_private=device;bio->bi_end_io=btrfs_end_super_write;-__bio_add_page(bio,page,BTRFS_SUPER_INFO_SIZE,-offset_in_page(bytenr));+bio_add_folio_nofail(bio,folio,BTRFS_SUPER_INFO_SIZE,+offset_in_folio(folio,bytenr));/**WeFUAonlythefirstsuperblock.Theothersweallowto
@@ -3842,7 +3839,7 @@ static int wait_dev_supers(struct btrfs_device *device, int max_mirrors)max_mirrors=BTRFS_SUPER_MIRROR_MAX;for(i=0;i<max_mirrors;i++){-structpage*page;+structfolio*folio;ret=btrfs_sb_log_location(device,i,READ,&bytenr);if(ret==-ENOENT){
@@ -3857,27 +3854,23 @@ static int wait_dev_supers(struct btrfs_device *device, int max_mirrors)device->commit_total_bytes)break;-page=find_get_page(device->bdev->bd_inode->i_mapping,-bytenr>>PAGE_SHIFT);-if(!page){+folio=bdev_get_folio(device->bdev,bytenr,0,0);+if(!IS_ERR(folio)){errors++;if(i==0)primary_failed=true;continue;}/* Page is submitted locked and unlocked once the IO completes */-wait_on_page_locked(page);-if(PageError(page)){+folio_wait_locked(folio);+if(folio_test_error(folio)){errors++;if(i==0)primary_failed=true;}-/* Drop our reference */-put_page(page);--/* Drop the reference from the writing run */-put_page(page);+/* Drop our reference and the reference from the writing run */+folio_put_refs(folio,2);}/* log error, force error return */
@@ -1230,16 +1230,16 @@ int btrfs_open_devices(struct btrfs_fs_devices *fs_devices,voidbtrfs_release_disk_super(structbtrfs_super_block*super){-structpage*page=virt_to_page(super);+structfolio*folio=virt_to_folio(super);-put_page(page);+folio_put(folio);}staticstructbtrfs_super_block*btrfs_read_disk_super(structblock_device*bdev,u64bytenr,u64bytenr_orig){structbtrfs_super_block*disk_super;-structpage*page;+structfolio*folio;void*p;pgoff_tindex;
@@ -1257,15 +1257,14 @@ static struct btrfs_super_block *btrfs_read_disk_super(struct block_device *bdevreturnERR_PTR(-EINVAL);/* pull in the page with our super */-page=read_cache_page_gfp(bdev->bd_inode->i_mapping,index,GFP_KERNEL);+folio=bdev_read_folio(bdev,index);+if(IS_ERR(folio))+returnERR_CAST(folio);-if(IS_ERR(page))-returnERR_CAST(page);--p=page_address(page);+p=folio_address(folio);/* align our pointer to the offset of the super block */-disk_super=p+offset_in_page(bytenr);+disk_super=p+offset_in_folio(folio,bytenr);if(btrfs_super_bytenr(disk_super)!=bytenr_orig||btrfs_super_magic(disk_super)!=BTRFS_MAGIC){
From: Yu Kuai <redacted>
On the one hand covert to use folio while reading bdev inode, on the
other hand prevent to access bd_inode directly.
Also do some cleanup that there is no need for two for loop, and remove
local array pages.
Signed-off-by: Yu Kuai <redacted>
---
fs/cramfs/inode.c | 36 +++++++++++++-----------------------
1 file changed, 13 insertions(+), 23 deletions(-)
@@ -290,7 +290,6 @@ int jbd2_journal_recover(journal_t *journal)structrecovery_infoinfo;errseq_twb_err;-structaddress_space*mapping;memset(&info,0,sizeof(info));sb=journal->j_superblock;
@@ -309,8 +308,7 @@ int jbd2_journal_recover(journal_t *journal)}wb_err=0;-mapping=journal->j_fs_dev->bd_inode->i_mapping;-errseq_check_and_advance(&mapping->wb_err,&wb_err);+bdev_wb_err_check_and_advance(journal->j_fs_dev,&wb_err);err=do_one_pass(journal,&info,PASS_SCAN);if(!err)err=do_one_pass(journal,&info,PASS_REVOKE);
@@ -334,7 +332,7 @@ int jbd2_journal_recover(journal_t *journal)err2=sync_blockdev(journal->j_fs_dev);if(!err)err=err2;-err2=errseq_check_and_advance(&mapping->wb_err,&wb_err);+err2=bdev_wb_err_check_and_advance(journal->j_fs_dev,&wb_err);if(!err)err=err2;/* Make sure all replayed data is on permanent storage */
From: Yu Kuai <redacted>
Unlike __bread_gfp(), ext4 has special handing while reading sb block:
1) __GFP_NOFAIL is not set, and memory allocation can fail;
2) If buffer write failed before, set buffer uptodate and don't read
block from disk;
3) REQ_META is set for all IO, and REQ_PRIO is set for reading xattr;
4) If failed, return error ptr instead of NULL;
This patch add a new helper __bread_gfp2() that will match above 2 and 3(
1 will be used, and 4 will still be encapsulated by ext4), and prepare to
prevent calling mapping_gfp_constraint() directly on bd_inode->i_mapping
in ext4.
Signed-off-by: Yu Kuai <redacted>
---
fs/buffer.c | 68 ++++++++++++++++++++++++++-----------
include/linux/buffer_head.h | 18 +++++++++-
2 files changed, 65 insertions(+), 21 deletions(-)
From: Yu Kuai <redacted>
Remove __ext4_sb_bread_gfp() and ext4_buffer_uptodate() that is defined
by ext4, and convert to use common helper __bread_gfp2() and
buffer_uptodate_or_error().
Signed-off-by: Yu Kuai <redacted>
Reviewed-by: Jan Kara <jack@suse.cz>
---
fs/ext4/ext4.h | 13 -------------
fs/ext4/inode.c | 8 ++++----
fs/ext4/super.c | 45 ++++++++++-----------------------------------
fs/ext4/symlink.c | 2 +-
4 files changed, 15 insertions(+), 53 deletions(-)
@@ -915,7 +915,7 @@ int ext4_bread_batch(struct inode *inode, ext4_lblk_t block, int bh_count,for(i=0;i<bh_count;i++)/* Note that NULL bhs[i] is valid because of holes. */-if(bhs[i]&&!ext4_buffer_uptodate(bhs[i]))+if(bhs[i]&&!buffer_uptodate_or_error(bhs[i]))ext4_read_bh_lock(bhs[i],REQ_META|REQ_PRIO,false);if(!wait)
@@ -4392,11 +4392,11 @@ static int __ext4_get_inode_loc(struct super_block *sb, unsigned long ino,bh=sb_getblk(sb,block);if(unlikely(!bh))return-ENOMEM;-if(ext4_buffer_uptodate(bh))+if(buffer_uptodate_or_error(bh))gotohas_buffer;lock_buffer(bh);-if(ext4_buffer_uptodate(bh)){+if(buffer_uptodate_or_error(bh)){/* Someone brought it uptodate while we waited */unlock_buffer(bh);gotohas_buffer;
From: Yu Kuai <redacted>
block_device_ejected() is added by commit bdfe0cbd746a ("Revert
"ext4: remove block_device_ejected"") in 2015. At that time 'bdi->wb'
is destroyed synchronized from del_gendisk(), hence if ext4 is still
mounted, and then mark_buffer_dirty() will reference destroyed 'wb'.
However, such problem doesn't exist anymore:
- commit d03f6cdc1fc4 ("block: Dynamically allocate and refcount
backing_dev_info") switch bdi to use refcounting;
- commit 13eec2363ef0 ("fs: Get proper reference for s_bdi"), will grab
additional reference of bdi while mounting, so that 'bdi->wb' will not
be destroyed until generic_shutdown_super().
Hence remove this dead function block_device_ejected().
Signed-off-by: Yu Kuai <redacted>
Reviewed-by: Jan Kara <jack@suse.cz>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
fs/ext4/super.c | 18 ------------------
1 file changed, 18 deletions(-)
From: Matthew Wilcox <willy@infradead.org> Date: 2023-12-23 17:33:13
On Thu, Dec 21, 2023 at 04:57:04PM +0800, Yu Kuai wrote:
quoted hunk
@@ -3674,16 +3670,17 @@ struct btrfs_super_block *btrfs_read_dev_one_super(struct block_device *bdev, * Drop the page of the primary superblock, so later read will * always read from the device. */- invalidate_inode_pages2_range(mapping,- bytenr >> PAGE_SHIFT,+ invalidate_bdev_range(bdev, bytenr >> PAGE_SHIFT, (bytenr + BTRFS_SUPER_INFO_SIZE) >> PAGE_SHIFT); }- page = read_cache_page_gfp(mapping, bytenr >> PAGE_SHIFT, GFP_NOFS);- if (IS_ERR(page))- return ERR_CAST(page);+ nofs_flag = memalloc_nofs_save();+ folio = bdev_read_folio(bdev, bytenr);+ memalloc_nofs_restore(nofs_flag);
This is the wrong way to use memalloc_nofs_save/restore. They should be
used at the point that the filesystem takes/releases whatever lock is
also used during reclaim. I don't know btrfs well enough to suggest
what lock is missing these annotations.
From: Kent Overstreet <kent.overstreet@linux.dev> Date: 2023-12-23 18:39:34
On Sat, Dec 23, 2023 at 05:31:55PM +0000, Matthew Wilcox wrote:
On Thu, Dec 21, 2023 at 04:57:04PM +0800, Yu Kuai wrote:
quoted
@@ -3674,16 +3670,17 @@ struct btrfs_super_block *btrfs_read_dev_one_super(struct block_device *bdev, * Drop the page of the primary superblock, so later read will * always read from the device. */- invalidate_inode_pages2_range(mapping,- bytenr >> PAGE_SHIFT,+ invalidate_bdev_range(bdev, bytenr >> PAGE_SHIFT, (bytenr + BTRFS_SUPER_INFO_SIZE) >> PAGE_SHIFT); }- page = read_cache_page_gfp(mapping, bytenr >> PAGE_SHIFT, GFP_NOFS);- if (IS_ERR(page))- return ERR_CAST(page);+ nofs_flag = memalloc_nofs_save();+ folio = bdev_read_folio(bdev, bytenr);+ memalloc_nofs_restore(nofs_flag);
This is the wrong way to use memalloc_nofs_save/restore. They should be
used at the point that the filesystem takes/releases whatever lock is
also used during reclaim. I don't know btrfs well enough to suggest
what lock is missing these annotations.
Yes, but considering this is a cross-filesystem cleanup I wouldn't want
to address that in this patchset. And the easier, more incremental
approach for the conversion would be to first convert every GFP_NOFS
usage to memalloc_nofs_save() like this patch does, as small local
changes, and then let the btrfs people combine them and move them to the
approproate location in a separate patchstet.
This function uses invalidate_inode_pages2() while invalidate_bdev() ends
up using mapping_try_invalidate() and there are subtle behavioral
differences between these two (for example invalidate_inode_pages2() tries
to clean dirty pages using the ->launder_folio method). So I think you'll
need helper like invalidate_bdev2() for this.
Honza
--
Jan Kara [off-list ref]
SUSE Labs, CR
From: Jan Kara <jack@suse.cz> Date: 2024-01-04 11:28:57
On Thu 21-12-23 16:56:59, Yu Kuai wrote:
From: Yu Kuai <redacted>
On the one hand covert to use folio while reading bdev inode, on the
other hand prevent to access bd_inode directly.
Signed-off-by: Yu Kuai <redacted>
...
quoted hunk
+ for (p = folio_address(folio); p < max; p++) if (*p != -1UL) {- lock_page(page);- memset(page_address(page), 0xff, PAGE_SIZE);- set_page_dirty(page);- unlock_page(page);- balance_dirty_pages_ratelimited(mapping);+ folio_lock(folio);+ memset(folio_address(folio), 0xff,+ folio_size(folio));+ folio_mark_dirty(folio);+ folio_unlock(folio);+ bdev_balance_dirty_pages_ratelimited(bdev);
Rather then creating this bdev_balance_dirty_pages_ratelimited() just for
MTD perhaps we can have here (and in other functions):
...
mapping = folio_mapping(folio);
folio_unlock(folio);
if (mapping)
balance_dirty_pages_ratelimited(mapping);
What do you think? Because when we are working with the folios it is rather
natural to use their mapping for dirty balancing?
Honza
--
Jan Kara [off-list ref]
SUSE Labs, CR
From: Jan Kara <jack@suse.cz> Date: 2024-01-04 11:50:05
On Sat 23-12-23 17:31:55, Matthew Wilcox wrote:
On Thu, Dec 21, 2023 at 04:57:04PM +0800, Yu Kuai wrote:
quoted
@@ -3674,16 +3670,17 @@ struct btrfs_super_block *btrfs_read_dev_one_super(struct block_device *bdev, * Drop the page of the primary superblock, so later read will * always read from the device. */- invalidate_inode_pages2_range(mapping,- bytenr >> PAGE_SHIFT,+ invalidate_bdev_range(bdev, bytenr >> PAGE_SHIFT, (bytenr + BTRFS_SUPER_INFO_SIZE) >> PAGE_SHIFT); }- page = read_cache_page_gfp(mapping, bytenr >> PAGE_SHIFT, GFP_NOFS);- if (IS_ERR(page))- return ERR_CAST(page);+ nofs_flag = memalloc_nofs_save();+ folio = bdev_read_folio(bdev, bytenr);+ memalloc_nofs_restore(nofs_flag);
This is the wrong way to use memalloc_nofs_save/restore. They should be
used at the point that the filesystem takes/releases whatever lock is
also used during reclaim. I don't know btrfs well enough to suggest
what lock is missing these annotations.
In principle I agree with you but in this particular case I agree the ask
is just too big. I suspect it is one of btrfs btree locks or maybe
chunk_mutex but I doubt even btrfs developers know and maybe it is just a
cargo cult. And it is not like this would be the first occurence of this
anti-pattern in btrfs - see e.g. device_list_add(), add_missing_dev(),
btrfs_destroy_delalloc_inodes() (here the wrapping around
invalidate_inode_pages2() looks really weird), and many others...
Honza
--
Jan Kara [off-list ref]
SUSE Labs, CR
From: Jan Kara <jack@suse.cz> Date: 2024-01-04 12:02:09
On Thu 21-12-23 16:58:26, Yu Kuai wrote:
From: Yu Kuai <redacted>
Avoid to access bd_inode directly, prepare to remove bd_inode from
block_device.
Signed-off-by: Yu Kuai <redacted>
I'm not erofs maintainer but IMO this is quite ugly and grows erofs_buf
unnecessarily. I'd rather store 'sb' pointer in erofs_buf and then do the
right thing in erofs_bread() which is the only place that seems to care
about the erofs_is_fscache_mode() distinction... Also blkszbits is then
trivially sb->s_blocksize_bits so it would all seem much more
straightforward.
Honza
From: Jan Kara <jack@suse.cz> Date: 2024-01-04 12:11:53
On Thu 21-12-23 16:58:46, Yu Kuai wrote:
From: Yu Kuai <redacted>
Avoid to access bd_inode directly, prepare to remove bd_inode from
block_device.
Signed-off-by: Yu Kuai <redacted>
Looks good to me. Feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
But note there are changes pending to this code for the coming merge window
so you'll have to rebase...
Honza
@@ -290,7 +290,6 @@ int jbd2_journal_recover(journal_t *journal)structrecovery_infoinfo;errseq_twb_err;-structaddress_space*mapping;memset(&info,0,sizeof(info));sb=journal->j_superblock;
@@ -309,8 +308,7 @@ int jbd2_journal_recover(journal_t *journal)}wb_err=0;-mapping=journal->j_fs_dev->bd_inode->i_mapping;-errseq_check_and_advance(&mapping->wb_err,&wb_err);+bdev_wb_err_check_and_advance(journal->j_fs_dev,&wb_err);err=do_one_pass(journal,&info,PASS_SCAN);if(!err)err=do_one_pass(journal,&info,PASS_REVOKE);
@@ -334,7 +332,7 @@ int jbd2_journal_recover(journal_t *journal)err2=sync_blockdev(journal->j_fs_dev);if(!err)err=err2;-err2=errseq_check_and_advance(&mapping->wb_err,&wb_err);+err2=bdev_wb_err_check_and_advance(journal->j_fs_dev,&wb_err);if(!err)err=err2;/* Make sure all replayed data is on permanent storage */
This function uses invalidate_inode_pages2() while invalidate_bdev() ends
up using mapping_try_invalidate() and there are subtle behavioral
differences between these two (for example invalidate_inode_pages2() tries
to clean dirty pages using the ->launder_folio method). So I think you'll
need helper like invalidate_bdev2() for this.
Thanks for reviewing this patch, I know the differenct between then,
what I don't understand is that why using invalidate_inode_pages2()
here. sync_blockdev() is just called and 0 is returned, I think in this
case it's safe to call invalidate_bdev() directly, or am I missing
other things?
Thanks,
Kuai
From: Jan Kara <jack@suse.cz> Date: 2024-01-04 12:22:17
On Thu 21-12-23 16:58:53, Yu Kuai wrote:
From: Yu Kuai <redacted>
Unlike __bread_gfp(), ext4 has special handing while reading sb block:
1) __GFP_NOFAIL is not set, and memory allocation can fail;
2) If buffer write failed before, set buffer uptodate and don't read
block from disk;
3) REQ_META is set for all IO, and REQ_PRIO is set for reading xattr;
4) If failed, return error ptr instead of NULL;
This patch add a new helper __bread_gfp2() that will match above 2 and 3(
1 will be used, and 4 will still be encapsulated by ext4), and prepare to
prevent calling mapping_gfp_constraint() directly on bd_inode->i_mapping
in ext4.
Signed-off-by: Yu Kuai <redacted>
I'm not enthusiastic about this but I guess it is as good as it gets
without larger cleanups in this area. So feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
From: Yu Kuai <redacted>
On the one hand covert to use folio while reading bdev inode, on the
other hand prevent to access bd_inode directly.
Signed-off-by: Yu Kuai <redacted>
...
quoted
+ for (p = folio_address(folio); p < max; p++) if (*p != -1UL) {- lock_page(page);- memset(page_address(page), 0xff, PAGE_SIZE);- set_page_dirty(page);- unlock_page(page);- balance_dirty_pages_ratelimited(mapping);+ folio_lock(folio);+ memset(folio_address(folio), 0xff,+ folio_size(folio));+ folio_mark_dirty(folio);+ folio_unlock(folio);+ bdev_balance_dirty_pages_ratelimited(bdev);
Rather then creating this bdev_balance_dirty_pages_ratelimited() just for
MTD perhaps we can have here (and in other functions):
...
mapping = folio_mapping(folio);
folio_unlock(folio);
if (mapping)
balance_dirty_pages_ratelimited(mapping);
What do you think? Because when we are working with the folios it is rather
natural to use their mapping for dirty balancing?
I think this is a great idea! And bdev_balance_dirty_pages_ratelimited()
can be removed as well.
Thanks,
Kuai
From: Yu Kuai <redacted>
Avoid to access bd_inode directly, prepare to remove bd_inode from
block_device.
Signed-off-by: Yu Kuai <redacted>
I'm not erofs maintainer but IMO this is quite ugly and grows erofs_buf
unnecessarily. I'd rather store 'sb' pointer in erofs_buf and then do the
right thing in erofs_bread() which is the only place that seems to care
about the erofs_is_fscache_mode() distinction... Also blkszbits is then
trivially sb->s_blocksize_bits so it would all seem much more
straightforward.
Thanks for your suggestion, I'll follow this unless Gao Xiang has other
suggestions.
Kuai
This function uses invalidate_inode_pages2() while invalidate_bdev() ends
up using mapping_try_invalidate() and there are subtle behavioral
differences between these two (for example invalidate_inode_pages2() tries
to clean dirty pages using the ->launder_folio method). So I think you'll
need helper like invalidate_bdev2() for this.
Thanks for reviewing this patch, I know the differenct between then,
what I don't understand is that why using invalidate_inode_pages2()
here.
Well, then the change in behavior should be at least noted in the
changelog.
sync_blockdev() is just called and 0 is returned, I think in this
case it's safe to call invalidate_bdev() directly, or am I missing
other things?
I still think there's a difference. invalidate_inode_pages2() also unmaps
memory mappings which mapping_try_invalidate() does not do. That being said
in xen_update_blkif_status() we seem to be bringing up a virtual block
device so before this function is called, anybody would have hard time
using anything in it. But this definitely needs a confirmation from Xen
maintainers and a good documentation of the behavioral change in the
changelog.
Honza
--
Jan Kara [off-list ref]
SUSE Labs, CR
From: Yu Kuai <redacted>
Avoid to access bd_inode directly, prepare to remove bd_inode from
block_device.
Signed-off-by: Yu Kuai <redacted>
I'm not erofs maintainer but IMO this is quite ugly and grows erofs_buf
unnecessarily. I'd rather store 'sb' pointer in erofs_buf and then do the
right thing in erofs_bread() which is the only place that seems to care
about the erofs_is_fscache_mode() distinction... Also blkszbits is then
trivially sb->s_blocksize_bits so it would all seem much more
straightforward.
Thanks for your suggestion, I'll follow this unless Gao Xiang has other
suggestions.
Yes, that would be better, I'm fine with that. Yet in the future we
may support a seperate large dirblocksize more than block size, but
we could revisit later.
Thanks,
Gao Xiang
From: Christoph Hellwig <hch@infradead.org> Date: 2024-01-05 06:09:24
On Thu, Jan 04, 2024 at 12:06:31PM +0100, Jan Kara wrote:
This function uses invalidate_inode_pages2() while invalidate_bdev() ends
up using mapping_try_invalidate() and there are subtle behavioral
differences between these two (for example invalidate_inode_pages2() tries
to clean dirty pages using the ->launder_folio method). So I think you'll
need helper like invalidate_bdev2() for this.
That assues that the existing code actually does this intentionally,
which seems doubtful. But the change in behavior does not to be
documented and explained.
From: Christoph Hellwig <hch@infradead.org> Date: 2024-01-05 06:10:39
On Thu, Jan 04, 2024 at 12:28:55PM +0100, Jan Kara wrote:
What do you think? Because when we are working with the folios it is rather
natural to use their mapping for dirty balancing?
The real problem is that block2mtd pokes way to deep into block
internals.
I think the saviour here is Christians series to replace the bdev handle
with a struct file, which will allow to use the normal file write path
here and get rid of the entire layering volation.
On Thu, Jan 04, 2024 at 12:28:55PM +0100, Jan Kara wrote:
quoted
What do you think? Because when we are working with the folios it is rather
natural to use their mapping for dirty balancing?
The real problem is that block2mtd pokes way to deep into block
internals.
I think the saviour here is Christians series to replace the bdev handle
with a struct file, which will allow to use the normal file write path
here and get rid of the entire layering volation.
Yes, looks like lots of patches from this set is not needed anymore.
I'll stop sending v4 and just send some patches that is not related to
'bd_inode' separately.
Thanks,
Kuai
From: David Sterba <hidden> Date: 2024-04-10 17:36:08
On Thu, Jan 04, 2024 at 12:49:58PM +0100, Jan Kara wrote:
On Sat 23-12-23 17:31:55, Matthew Wilcox wrote:
quoted
On Thu, Dec 21, 2023 at 04:57:04PM +0800, Yu Kuai wrote:
quoted
@@ -3674,16 +3670,17 @@ struct btrfs_super_block *btrfs_read_dev_one_super(struct block_device *bdev, * Drop the page of the primary superblock, so later read will * always read from the device. */- invalidate_inode_pages2_range(mapping,- bytenr >> PAGE_SHIFT,+ invalidate_bdev_range(bdev, bytenr >> PAGE_SHIFT, (bytenr + BTRFS_SUPER_INFO_SIZE) >> PAGE_SHIFT); }- page = read_cache_page_gfp(mapping, bytenr >> PAGE_SHIFT, GFP_NOFS);- if (IS_ERR(page))- return ERR_CAST(page);+ nofs_flag = memalloc_nofs_save();+ folio = bdev_read_folio(bdev, bytenr);+ memalloc_nofs_restore(nofs_flag);
This is the wrong way to use memalloc_nofs_save/restore. They should be
used at the point that the filesystem takes/releases whatever lock is
also used during reclaim. I don't know btrfs well enough to suggest
what lock is missing these annotations.
In principle I agree with you but in this particular case I agree the ask
is just too big. I suspect it is one of btrfs btree locks or maybe
chunk_mutex but I doubt even btrfs developers know and maybe it is just a
cargo cult. And it is not like this would be the first occurence of this
anti-pattern in btrfs - see e.g. device_list_add(), add_missing_dev(),
btrfs_destroy_delalloc_inodes() (here the wrapping around
invalidate_inode_pages2() looks really weird), and many others...
The pattern is intentional and a temporary solution before we could
implement the scoped NOFS. Functions calling allocations get converted
from GFP_NOFS to GFP_KERNEL but in case they're called from a context
that either holds big locks or can recursively enter the filesystem then
it's protected by the memalloc calls. This should not be surprising.
What may not be obvious is which locks or kmalloc calling functions it
could be, this depends on the analysis of the function call chain and
usually there's enough evidence why it's needed.