| Message ID | 20251107144249.435029-9-libaokun@huaweicloud.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ext4: enable block size larger than page size | expand |
On 11/7/25 15:42, libaokun@huaweicloud.com wrote: > From: Baokun Li <libaokun1@huawei.com> > > In ext4_readdir(), page_cache_sync_readahead() is used to readahead mapped > physical blocks. With LBS support, this can lead to a negative right shift. > > To fix this, the page index is now calculated by first converting the > physical block number (pblk) to a file position (pos) before converting > it to a page index. Also, the correct number of pages to readahead is now > passed. > > Signed-off-by: Baokun Li <libaokun1@huawei.com> > Reviewed-by: Zhang Yi <yi.zhang@huawei.com> > Reviewed-by: Jan Kara <jack@suse.cz> > --- Minor general comments below. Reviewed-by: Pankaj Raghav <p.raghav@samsung.com> > fs/ext4/dir.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/fs/ext4/dir.c b/fs/ext4/dir.c > index d4164c507a90..256fe2c1d4c1 100644 > --- a/fs/ext4/dir.c > +++ b/fs/ext4/dir.c > @@ -192,13 +192,13 @@ static int ext4_readdir(struct file *file, struct dir_context *ctx) > continue; > } > if (err > 0) { > - pgoff_t index = map.m_pblk >> > - (PAGE_SHIFT - inode->i_blkbits); > + pgoff_t index = map.m_pblk << inode->i_blkbits >> > + PAGE_SHIFT; It will be nice to have some common helper for this operation. We do something similar in filemap.c as well. > if (!ra_has_index(&file->f_ra, index)) > page_cache_sync_readahead( > sb->s_bdev->bd_mapping, > - &file->f_ra, file, > - index, 1); > + &file->f_ra, file, index, > + 1 << EXT4_SB(sb)->s_min_folio_order); Just a personal opinion but it would be nice to have some variable for this instead of doing it inline? It could be defined along with index. unsigned long min_nr_pages = 1UL << EXT4_SB(sb)->s_min_folio_order; > file->f_ra.prev_pos = (loff_t)index << PAGE_SHIFT; > bh = ext4_bread(NULL, inode, map.m_lblk, 0); > if (IS_ERR(bh)) {
On 2025-11-10 20:26, Pankaj Raghav wrote: > On 11/7/25 15:42, libaokun@huaweicloud.com wrote: >> From: Baokun Li <libaokun1@huawei.com> >> >> In ext4_readdir(), page_cache_sync_readahead() is used to readahead mapped >> physical blocks. With LBS support, this can lead to a negative right shift. >> >> To fix this, the page index is now calculated by first converting the >> physical block number (pblk) to a file position (pos) before converting >> it to a page index. Also, the correct number of pages to readahead is now >> passed. >> >> Signed-off-by: Baokun Li <libaokun1@huawei.com> >> Reviewed-by: Zhang Yi <yi.zhang@huawei.com> >> Reviewed-by: Jan Kara <jack@suse.cz> >> --- > Minor general comments below. > > Reviewed-by: Pankaj Raghav <p.raghav@samsung.com> Thanks for the review! > >> fs/ext4/dir.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/fs/ext4/dir.c b/fs/ext4/dir.c >> index d4164c507a90..256fe2c1d4c1 100644 >> --- a/fs/ext4/dir.c >> +++ b/fs/ext4/dir.c >> @@ -192,13 +192,13 @@ static int ext4_readdir(struct file *file, struct dir_context *ctx) >> continue; >> } >> if (err > 0) { >> - pgoff_t index = map.m_pblk >> >> - (PAGE_SHIFT - inode->i_blkbits); >> + pgoff_t index = map.m_pblk << inode->i_blkbits >> >> + PAGE_SHIFT; > It will be nice to have some common helper for this operation. We do something > similar in filemap.c as well. In patch 10, we introduced a macro to handle the conversion from lblk to page index. In this particular case, though, it is pblk, which has a different value range compared to lblk. As this is the only instance in the code, I applied a standalone modification here. > >> if (!ra_has_index(&file->f_ra, index)) >> page_cache_sync_readahead( >> sb->s_bdev->bd_mapping, >> - &file->f_ra, file, >> - index, 1); >> + &file->f_ra, file, index, >> + 1 << EXT4_SB(sb)->s_min_folio_order); > Just a personal opinion but it would be nice to have some variable for this instead of doing it > inline? It could be defined along with index. > > unsigned long min_nr_pages = 1UL << EXT4_SB(sb)->s_min_folio_order; > Thank you for the suggestion. This is indeed a bit easier to understand, but the space here is limited and the variable is only used once, so I prefer the current direct style. Cheers, Baokun >> file->f_ra.prev_pos = (loff_t)index << PAGE_SHIFT; >> bh = ext4_bread(NULL, inode, map.m_lblk, 0); >> if (IS_ERR(bh)) { >
diff --git a/fs/ext4/dir.c b/fs/ext4/dir.c index d4164c507a90..256fe2c1d4c1 100644 --- a/fs/ext4/dir.c +++ b/fs/ext4/dir.c @@ -192,13 +192,13 @@ static int ext4_readdir(struct file *file, struct dir_context *ctx) continue; } if (err > 0) { - pgoff_t index = map.m_pblk >> - (PAGE_SHIFT - inode->i_blkbits); + pgoff_t index = map.m_pblk << inode->i_blkbits >> + PAGE_SHIFT; if (!ra_has_index(&file->f_ra, index)) page_cache_sync_readahead( sb->s_bdev->bd_mapping, - &file->f_ra, file, - index, 1); + &file->f_ra, file, index, + 1 << EXT4_SB(sb)->s_min_folio_order); file->f_ra.prev_pos = (loff_t)index << PAGE_SHIFT; bh = ext4_bread(NULL, inode, map.m_lblk, 0); if (IS_ERR(bh)) {