From: Yue Hu <[email protected]>
linux/fs.h has a wrapper for this operation.
Signed-off-by: Yue Hu <[email protected]>
---
fs/erofs/data.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/erofs/data.c b/fs/erofs/data.c
index 5bd0c956a142..7e8baf56faa5 100644
--- a/fs/erofs/data.c
+++ b/fs/erofs/data.c
@@ -380,7 +380,7 @@ static ssize_t erofs_file_read_iter(struct kiocb *iocb, struct iov_iter *to)
if (bdev)
blksize_mask = bdev_logical_block_size(bdev) - 1;
else
- blksize_mask = (1 << inode->i_blkbits) - 1;
+ blksize_mask = i_blocksize(inode) - 1;
if ((iocb->ki_pos | iov_iter_count(to) |
iov_iter_alignment(to)) & blksize_mask)
--
2.17.1
On 2023/3/6 15:55, Yue Hu wrote:
> From: Yue Hu <[email protected]>
>
> linux/fs.h has a wrapper for this operation.
>
> Signed-off-by: Yue Hu <[email protected]>
Reviewed-by: Gao Xiang <[email protected]>
Thanks,
Gao Xiang
> ---
> fs/erofs/data.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/erofs/data.c b/fs/erofs/data.c
> index 5bd0c956a142..7e8baf56faa5 100644
> --- a/fs/erofs/data.c
> +++ b/fs/erofs/data.c
> @@ -380,7 +380,7 @@ static ssize_t erofs_file_read_iter(struct kiocb *iocb, struct iov_iter *to)
> if (bdev)
> blksize_mask = bdev_logical_block_size(bdev) - 1;
> else
> - blksize_mask = (1 << inode->i_blkbits) - 1;
> + blksize_mask = i_blocksize(inode) - 1;
>
> if ((iocb->ki_pos | iov_iter_count(to) |
> iov_iter_alignment(to)) & blksize_mask)
> @@ -380,7 +380,7 @@ static ssize_t erofs_file_read_iter(struct kiocb *iocb, struct iov_iter *to)
> if (bdev)
> blksize_mask = bdev_logical_block_size(bdev) - 1;
> else
> - blksize_mask = (1 << inode->i_blkbits) - 1;
> + blksize_mask = i_blocksize(inode) - 1;
Since the mask is to be obtained here, is it more appropriate to use GENMASK(inode->i_blkbits - 1, 0)?
Thx,
Yangtao
On Thu, 9 Mar 2023 15:15:15 +0800
Yangtao Li <[email protected]> wrote:
> > @@ -380,7 +380,7 @@ static ssize_t erofs_file_read_iter(struct kiocb *iocb, struct iov_iter *to)
> > if (bdev)
> > blksize_mask = bdev_logical_block_size(bdev) - 1;
> > else
> > - blksize_mask = (1 << inode->i_blkbits) - 1;
> > + blksize_mask = i_blocksize(inode) - 1;
>
> Since the mask is to be obtained here, is it more appropriate to use GENMASK(inode->i_blkbits - 1, 0)?
It should be another change independently to this patch. rt?
>
> Thx,
> Yangtao
On 3/9/23 3:15 PM, Yangtao Li wrote:
>> @@ -380,7 +380,7 @@ static ssize_t erofs_file_read_iter(struct kiocb *iocb, struct iov_iter *to)
>> if (bdev)
>> blksize_mask = bdev_logical_block_size(bdev) - 1;
>> else
>> - blksize_mask = (1 << inode->i_blkbits) - 1;
>> + blksize_mask = i_blocksize(inode) - 1;
>
> Since the mask is to be obtained here, is it more appropriate to use GENMASK(inode->i_blkbits - 1, 0)?
FYI it seems that GENMASK macro is widely used in driver and arch code
base, while it's rarely used in fs, except for f2fs.
--
Thanks,
Jingbo
> FYI it seems that GENMASK macro is widely used in driver and arch code base, while it's rarely used in fs, except for f2fs.
I think the following usage can be changed to bitmap api, just like in f2fs?
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=c4ca1f7164734a1baf40d4ff1552172a07d4fc4d
fs/erofs/fscache.c:135: unsigned long flags = 1 << NETFS_SREQ_ONDEMAND;
fs/erofs/internal.h:250:#define SECTORS_PER_BLOCK (1 << SECTORS_PER_BLOCK)
fs/erofs/internal.h:252:#define EROFS_BLKSIZ (1 << LOG_BLOCK_SIZE)
fs/erofs/internal.h:354: return (value >> bit) & ((1 << bits) - 1);
fs/erofs/zmap.c:66: ((1 << Z_EROFS_VLE_DI_CLUSTER_TYPE_BITS) - 1);
fs/erofs/zmap.c:69: m->clusterofs = 1 << vi->z_logical_clusterbits;
fs/erofs/zmap.c:114: const unsigned int lomask = (1 << lclusterbits) - 1;
fs/erofs/zmap.c:141: const unsigned int lomask = (1 << lclusterbits) - 1;
fs/erofs/zmap.c:147: if (1 << amortizedshift == 4)
fs/erofs/zmap.c:149: else if (1 << amortizedshift == 2 && lclusterbits == 12)
fs/erofs/zmap.c:169: m->clusterofs = 1 << lclusterbits;
fs/erofs/zmap.c:291: pos += lcn * (1 << amortizedshift);
fs/erofs/zmap.c:409: m->compressedblks = 1 << (lclusterbits - LOG_BLOCK_SIZE);
fs/erofs/zmap.c:457: m->clusterofs != 1 << lclusterbits);
fs/erofs/zmap.c:497: endoff = ofs & ((1 << lclusterbits) - 1);
fs/erofs/erofs_fs.h:120: ((1 << (EROFS_I_DATALAYOUT_BIT + EROFS_I_DATALAYOUT_BITS)) - 1)
fs/erofs/erofs_fs.h:279:#define Z_EROFS_ALL_COMPR_ALGS ((1 << Z_EROFS_COMPRESSION_MAX) - 1)
fs/erofs/erofs_fs.h:377:#define Z_EROFS_VLE_DI_PARTIAL_REF (1 << 15)
fs/erofs/erofs_fs.h:384:#define Z_EROFS_VLE_DI_D0_CBLKCNT (1 << 11)
fs/erofs/erofs_fs.h:427: .h_clusterbits = 1 << Z_EROFS_FRAGMENT_INODE_BIT
fs/erofs/data.c:379: blksize_mask = (1 << inode->i_blkbits) - 1;
fs/erofs/zdata.c:133:#define Z_EROFS_PAGE_EIO (1 << 30)
Thx,
Yangtao
On 2023/3/9 15:37, Yue Hu wrote:
> On Thu, 9 Mar 2023 15:15:15 +0800
> Yangtao Li <[email protected]> wrote:
>
>>> @@ -380,7 +380,7 @@ static ssize_t erofs_file_read_iter(struct kiocb *iocb, struct iov_iter *to)
>>> if (bdev)
>>> blksize_mask = bdev_logical_block_size(bdev) - 1;
>>> else
>>> - blksize_mask = (1 << inode->i_blkbits) - 1;
>>> + blksize_mask = i_blocksize(inode) - 1;
>>
>> Since the mask is to be obtained here, is it more appropriate to use GENMASK(inode->i_blkbits - 1, 0)?
>
> It should be another change independently to this patch. rt?
I'd suggest that keep to use (i_blocksize(inode) - 1) here, for example:
https://git.kernel.org/pub/scm/linux/kernel/git/xiang/erofs.git/tree/fs/gfs2/bmap.c#n963
Thanks,
Gao Xiang
>
>>
>> Thx,
>> Yangtao
On 2023/3/9 15:42, Yangtao Li wrote:
>> FYI it seems that GENMASK macro is widely used in driver and arch code base, while it's rarely used in fs, except for f2fs.
>
> I think the following usage can be changed to bitmap api, just like in f2fs?
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=c4ca1f7164734a1baf40d4ff1552172a07d4fc4d
>
> fs/erofs/fscache.c:135: unsigned long flags = 1 << NETFS_SREQ_ONDEMAND;
> fs/erofs/internal.h:250:#define SECTORS_PER_BLOCK (1 << SECTORS_PER_BLOCK)
> fs/erofs/internal.h:252:#define EROFS_BLKSIZ (1 << LOG_BLOCK_SIZE)
> fs/erofs/internal.h:354: return (value >> bit) & ((1 << bits) - 1);
> fs/erofs/zmap.c:66: ((1 << Z_EROFS_VLE_DI_CLUSTER_TYPE_BITS) - 1);
> fs/erofs/zmap.c:69: m->clusterofs = 1 << vi->z_logical_clusterbits;
> fs/erofs/zmap.c:114: const unsigned int lomask = (1 << lclusterbits) - 1;
> fs/erofs/zmap.c:141: const unsigned int lomask = (1 << lclusterbits) - 1;
> fs/erofs/zmap.c:147: if (1 << amortizedshift == 4)
> fs/erofs/zmap.c:149: else if (1 << amortizedshift == 2 && lclusterbits == 12)
> fs/erofs/zmap.c:169: m->clusterofs = 1 << lclusterbits;
> fs/erofs/zmap.c:291: pos += lcn * (1 << amortizedshift);
> fs/erofs/zmap.c:409: m->compressedblks = 1 << (lclusterbits - LOG_BLOCK_SIZE);
> fs/erofs/zmap.c:457: m->clusterofs != 1 << lclusterbits);
> fs/erofs/zmap.c:497: endoff = ofs & ((1 << lclusterbits) - 1);
> fs/erofs/erofs_fs.h:120: ((1 << (EROFS_I_DATALAYOUT_BIT + EROFS_I_DATALAYOUT_BITS)) - 1)
> fs/erofs/erofs_fs.h:279:#define Z_EROFS_ALL_COMPR_ALGS ((1 << Z_EROFS_COMPRESSION_MAX) - 1)
> fs/erofs/erofs_fs.h:377:#define Z_EROFS_VLE_DI_PARTIAL_REF (1 << 15)
> fs/erofs/erofs_fs.h:384:#define Z_EROFS_VLE_DI_D0_CBLKCNT (1 << 11)
> fs/erofs/erofs_fs.h:427: .h_clusterbits = 1 << Z_EROFS_FRAGMENT_INODE_BIT
> fs/erofs/data.c:379: blksize_mask = (1 << inode->i_blkbits) - 1;
> fs/erofs/zdata.c:133:#define Z_EROFS_PAGE_EIO (1 << 30)
>
Is there some benefit to use these? BIT(1) vs 1 << 1? also almost all
filesystems rarely use such APIs honestly.
Thanks,
Gao Xiang
> Thx,
> Yangtao
On 2023/3/6 15:55, Yue Hu wrote:
> From: Yue Hu <[email protected]>
>
> linux/fs.h has a wrapper for this operation.
>
> Signed-off-by: Yue Hu <[email protected]>
Reviewed-by: Chao Yu <[email protected]>
Thanks,