| Message ID | 20240514152208.26935-1-jth@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | zonefs: move super block reading from page to folio | expand |
On Tue, May 14, 2024 at 05:22:08PM +0200, Johannes Thumshirn wrote: > From: Johannes Thumshirn <johannes.thumshirn@wdc.com> > > Move reading of the on-disk superblock from page to kmalloc()ed memory. > > Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> Looks fine. Reviewed-by: Bill O'Donnell <bodonnel@redhat.com> > --- > fs/zonefs/super.c | 30 +++++++++++++++--------------- > 1 file changed, 15 insertions(+), 15 deletions(-) > > diff --git a/fs/zonefs/super.c b/fs/zonefs/super.c > index faf1eb87895d..ebea18da6759 100644 > --- a/fs/zonefs/super.c > +++ b/fs/zonefs/super.c > @@ -1111,28 +1111,28 @@ static int zonefs_read_super(struct super_block *sb) > struct zonefs_sb_info *sbi = ZONEFS_SB(sb); > struct zonefs_super *super; > u32 crc, stored_crc; > - struct page *page; > struct bio_vec bio_vec; > struct bio bio; > + struct folio *folio; > int ret; > > - page = alloc_page(GFP_KERNEL); > - if (!page) > + super = kzalloc(ZONEFS_SUPER_SIZE, GFP_KERNEL); > + if (!super) > return -ENOMEM; > > + folio = virt_to_folio(super); > bio_init(&bio, sb->s_bdev, &bio_vec, 1, REQ_OP_READ); > bio.bi_iter.bi_sector = 0; > - __bio_add_page(&bio, page, PAGE_SIZE, 0); > + bio_add_folio_nofail(&bio, folio, ZONEFS_SUPER_SIZE, > + offset_in_folio(folio, super)); > > ret = submit_bio_wait(&bio); > if (ret) > - goto free_page; > - > - super = page_address(page); > + goto free_super; > > ret = -EINVAL; > if (le32_to_cpu(super->s_magic) != ZONEFS_MAGIC) > - goto free_page; > + goto free_super; > > stored_crc = le32_to_cpu(super->s_crc); > super->s_crc = 0; > @@ -1140,14 +1140,14 @@ static int zonefs_read_super(struct super_block *sb) > if (crc != stored_crc) { > zonefs_err(sb, "Invalid checksum (Expected 0x%08x, got 0x%08x)", > crc, stored_crc); > - goto free_page; > + goto free_super; > } > > sbi->s_features = le64_to_cpu(super->s_features); > if (sbi->s_features & ~ZONEFS_F_DEFINED_FEATURES) { > zonefs_err(sb, "Unknown features set 0x%llx\n", > sbi->s_features); > - goto free_page; > + goto free_super; > } > > if (sbi->s_features & ZONEFS_F_UID) { > @@ -1155,7 +1155,7 @@ static int zonefs_read_super(struct super_block *sb) > le32_to_cpu(super->s_uid)); > if (!uid_valid(sbi->s_uid)) { > zonefs_err(sb, "Invalid UID feature\n"); > - goto free_page; > + goto free_super; > } > } > > @@ -1164,7 +1164,7 @@ static int zonefs_read_super(struct super_block *sb) > le32_to_cpu(super->s_gid)); > if (!gid_valid(sbi->s_gid)) { > zonefs_err(sb, "Invalid GID feature\n"); > - goto free_page; > + goto free_super; > } > } > > @@ -1173,14 +1173,14 @@ static int zonefs_read_super(struct super_block *sb) > > if (memchr_inv(super->s_reserved, 0, sizeof(super->s_reserved))) { > zonefs_err(sb, "Reserved area is being used\n"); > - goto free_page; > + goto free_super; > } > > import_uuid(&sbi->s_uuid, super->s_uuid); > ret = 0; > > -free_page: > - __free_page(page); > +free_super: > + kfree(super); > > return ret; > } > -- > 2.35.3 > >
On Tue, May 14, 2024 at 05:22:08PM +0200, Johannes Thumshirn wrote: > From: Johannes Thumshirn <johannes.thumshirn@wdc.com> > > Move reading of the on-disk superblock from page to kmalloc()ed memory. No, this is wrong. > + super = kzalloc(ZONEFS_SUPER_SIZE, GFP_KERNEL); > + if (!super) > return -ENOMEM; > > + folio = virt_to_folio(super); This will stop working at some point. It'll return NULL once we get to the memdesc future (because the memdesc will be a slab, not a folio). > bio_init(&bio, sb->s_bdev, &bio_vec, 1, REQ_OP_READ); > bio.bi_iter.bi_sector = 0; > - __bio_add_page(&bio, page, PAGE_SIZE, 0); > + bio_add_folio_nofail(&bio, folio, ZONEFS_SUPER_SIZE, > + offset_in_folio(folio, super)); It also doesn't solve the problem of trying to read 4KiB from a device with 16KiB sectors. We'll have to fail the bio because there isn't enough memory in the bio to store one block. I think the right way to handle this is to call read_mapping_folio(). That will allocate a folio in the page cache for you (obeying the minimum folio size). Then you can examine the contents. It should actually remove code from zonefs. Don't forget to call folio_put() when you're done with it (either at unmount or at the end of mount if you copy what you need elsewhere).
On 23.05.24 03:42, Matthew Wilcox wrote: > On Tue, May 14, 2024 at 05:22:08PM +0200, Johannes Thumshirn wrote: >> From: Johannes Thumshirn <johannes.thumshirn@wdc.com> >> >> Move reading of the on-disk superblock from page to kmalloc()ed memory. > > No, this is wrong. > >> + super = kzalloc(ZONEFS_SUPER_SIZE, GFP_KERNEL); >> + if (!super) >> return -ENOMEM; >> >> + folio = virt_to_folio(super); > > This will stop working at some point. It'll return NULL once we get > to the memdesc future (because the memdesc will be a slab, not a folio). > >> bio_init(&bio, sb->s_bdev, &bio_vec, 1, REQ_OP_READ); >> bio.bi_iter.bi_sector = 0; >> - __bio_add_page(&bio, page, PAGE_SIZE, 0); >> + bio_add_folio_nofail(&bio, folio, ZONEFS_SUPER_SIZE, >> + offset_in_folio(folio, super)); > > It also doesn't solve the problem of trying to read 4KiB from a device > with 16KiB sectors. We'll have to fail the bio because there isn't > enough memory in the bio to store one block. > > I think the right way to handle this is to call read_mapping_folio(). > That will allocate a folio in the page cache for you (obeying the > minimum folio size). Then you can examine the contents. It should > actually remove code from zonefs. Don't forget to call folio_put() > when you're done with it (either at unmount or at the end of mount if > you copy what you need elsewhere). > Hmm but read mapping folio needs an inode for the address_space. Or does the block device inode work here?
On Thu, May 23, 2024 at 09:54:00AM +0000, Johannes Thumshirn wrote: > On 23.05.24 03:42, Matthew Wilcox wrote: > > I think the right way to handle this is to call read_mapping_folio(). > > That will allocate a folio in the page cache for you (obeying the > > minimum folio size). Then you can examine the contents. It should > > actually remove code from zonefs. Don't forget to call folio_put() > > when you're done with it (either at unmount or at the end of mount if > > you copy what you need elsewhere). > > Hmm but read mapping folio needs an inode for the address_space. Or does > the block device inode work here? Sorry, yes, should have been explicit. Read it using the bdev's address_space.
On 23.05.24 13:48, Matthew Wilcox wrote: > On Thu, May 23, 2024 at 09:54:00AM +0000, Johannes Thumshirn wrote: >> On 23.05.24 03:42, Matthew Wilcox wrote: >>> I think the right way to handle this is to call read_mapping_folio(). >>> That will allocate a folio in the page cache for you (obeying the >>> minimum folio size). Then you can examine the contents. It should >>> actually remove code from zonefs. Don't forget to call folio_put() >>> when you're done with it (either at unmount or at the end of mount if >>> you copy what you need elsewhere). >> >> Hmm but read mapping folio needs an inode for the address_space. Or does >> the block device inode work here? > > Sorry, yes, should have been explicit. Read it using the bdev's > address_space. > Ah OK then it's easy.
diff --git a/fs/zonefs/super.c b/fs/zonefs/super.c index faf1eb87895d..ebea18da6759 100644 --- a/fs/zonefs/super.c +++ b/fs/zonefs/super.c @@ -1111,28 +1111,28 @@ static int zonefs_read_super(struct super_block *sb) struct zonefs_sb_info *sbi = ZONEFS_SB(sb); struct zonefs_super *super; u32 crc, stored_crc; - struct page *page; struct bio_vec bio_vec; struct bio bio; + struct folio *folio; int ret; - page = alloc_page(GFP_KERNEL); - if (!page) + super = kzalloc(ZONEFS_SUPER_SIZE, GFP_KERNEL); + if (!super) return -ENOMEM; + folio = virt_to_folio(super); bio_init(&bio, sb->s_bdev, &bio_vec, 1, REQ_OP_READ); bio.bi_iter.bi_sector = 0; - __bio_add_page(&bio, page, PAGE_SIZE, 0); + bio_add_folio_nofail(&bio, folio, ZONEFS_SUPER_SIZE, + offset_in_folio(folio, super)); ret = submit_bio_wait(&bio); if (ret) - goto free_page; - - super = page_address(page); + goto free_super; ret = -EINVAL; if (le32_to_cpu(super->s_magic) != ZONEFS_MAGIC) - goto free_page; + goto free_super; stored_crc = le32_to_cpu(super->s_crc); super->s_crc = 0; @@ -1140,14 +1140,14 @@ static int zonefs_read_super(struct super_block *sb) if (crc != stored_crc) { zonefs_err(sb, "Invalid checksum (Expected 0x%08x, got 0x%08x)", crc, stored_crc); - goto free_page; + goto free_super; } sbi->s_features = le64_to_cpu(super->s_features); if (sbi->s_features & ~ZONEFS_F_DEFINED_FEATURES) { zonefs_err(sb, "Unknown features set 0x%llx\n", sbi->s_features); - goto free_page; + goto free_super; } if (sbi->s_features & ZONEFS_F_UID) { @@ -1155,7 +1155,7 @@ static int zonefs_read_super(struct super_block *sb) le32_to_cpu(super->s_uid)); if (!uid_valid(sbi->s_uid)) { zonefs_err(sb, "Invalid UID feature\n"); - goto free_page; + goto free_super; } } @@ -1164,7 +1164,7 @@ static int zonefs_read_super(struct super_block *sb) le32_to_cpu(super->s_gid)); if (!gid_valid(sbi->s_gid)) { zonefs_err(sb, "Invalid GID feature\n"); - goto free_page; + goto free_super; } } @@ -1173,14 +1173,14 @@ static int zonefs_read_super(struct super_block *sb) if (memchr_inv(super->s_reserved, 0, sizeof(super->s_reserved))) { zonefs_err(sb, "Reserved area is being used\n"); - goto free_page; + goto free_super; } import_uuid(&sbi->s_uuid, super->s_uuid); ret = 0; -free_page: - __free_page(page); +free_super: + kfree(super); return ret; }