diff mbox series

[v3,2/6] erofs: convert to use i_blockmask()

Message ID 20230309152127.41427-2-frank.li@vivo.com
State New
Headers
Series [v3,1/6] fs: add i_blockmask() |

Commit Message

李扬韬 March 9, 2023, 3:21 p.m. UTC 
  Use i_blockmask() to simplify code.

Signed-off-by: Yangtao Li <frank.li@vivo.com>
---
v3:
-none
v2:
-convert to i_blockmask()
 fs/erofs/data.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Al Viro March 10, 2023, 3:15 a.m. UTC | #1 
On Thu, Mar 09, 2023 at 11:21:23PM +0800, Yangtao Li wrote:
> Use i_blockmask() to simplify code.

Umm...  What's the branchpoint for that series?  Not the mainline -
there we have i_blocksize() open-coded...

> Signed-off-by: Yangtao Li <frank.li@vivo.com>
> ---
> v3:
> -none
> v2:
> -convert to i_blockmask()
>  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 7e8baf56faa5..e9d1869cd4b3 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 = i_blocksize(inode) - 1;
> +			blksize_mask = i_blockmask(inode);
>  
>  		if ((iocb->ki_pos | iov_iter_count(to) |
>  		     iov_iter_alignment(to)) & blksize_mask)
> -- 
> 2.25.1
>
Gao Xiang March 10, 2023, 3:42 a.m. UTC | #2 
Hi Al,

On 2023/3/10 11:15, Al Viro wrote:
> On Thu, Mar 09, 2023 at 11:21:23PM +0800, Yangtao Li wrote:
>> Use i_blockmask() to simplify code.
> 
> Umm...  What's the branchpoint for that series?  Not the mainline -
> there we have i_blocksize() open-coded...

Actually Yue Hu sent out a clean-up patch and I applied to -next for
almost a week and will be upstreamed for 6.3-rc2:

https://lore.kernel.org/r/a238dca1-256f-ae2f-4a33-e54861fe4ffb@kernel.org/T/#t

And then Yangtao would like to wrap this as a new VFS helper, I'm not
sure why it's necessary since it doesn't save a lot but anyway, I'm open
to it if VFS could have such new helper.

Thanks,
Gao Xiang

> 
>> Signed-off-by: Yangtao Li <frank.li@vivo.com>
>> ---
>> v3:
>> -none
>> v2:
>> -convert to i_blockmask()
>>   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 7e8baf56faa5..e9d1869cd4b3 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 = i_blocksize(inode) - 1;
>> +			blksize_mask = i_blockmask(inode);
>>   
>>   		if ((iocb->ki_pos | iov_iter_count(to) |
>>   		     iov_iter_alignment(to)) & blksize_mask)
>> -- 
>> 2.25.1
>>
Gao Xiang March 10, 2023, 3:47 a.m. UTC | #3 
On 2023/3/10 11:42, Gao Xiang wrote:
> Hi Al,
> 
> On 2023/3/10 11:15, Al Viro wrote:
>> On Thu, Mar 09, 2023 at 11:21:23PM +0800, Yangtao Li wrote:
>>> Use i_blockmask() to simplify code.
>>
>> Umm...  What's the branchpoint for that series?  Not the mainline -
>> there we have i_blocksize() open-coded...
> 
> Actually Yue Hu sent out a clean-up patch and I applied to -next for
> almost a week and will be upstreamed for 6.3-rc2:
> 
> https://lore.kernel.org/r/a238dca1-256f-ae2f-4a33-e54861fe4ffb@kernel.org/T/#t

Sorry this link:
https://lore.kernel.org/r/0261de31-e98b-85cd-80de-96af5a76e15c@linux.alibaba.com

Yangtao's suggestion was to use GENMASK, and I'm not sure it's a good way
since (i_blocksize(inode) - 1) is simple enough, and then it becomes like
this.

Thanks,
Gao Xiang


> 
> And then Yangtao would like to wrap this as a new VFS helper, I'm not
> sure why it's necessary since it doesn't save a lot but anyway, I'm open
> to it if VFS could have such new helper.
> 
> Thanks,
> Gao Xiang
> 
>>
>>> Signed-off-by: Yangtao Li <frank.li@vivo.com>
>>> ---
>>> v3:
>>> -none
>>> v2:
>>> -convert to i_blockmask()
>>>   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 7e8baf56faa5..e9d1869cd4b3 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 = i_blocksize(inode) - 1;
>>> +            blksize_mask = i_blockmask(inode);
>>>           if ((iocb->ki_pos | iov_iter_count(to) |
>>>                iov_iter_alignment(to)) & blksize_mask)
>>> -- 
>>> 2.25.1
>>>
李扬韬 March 10, 2023, 3:51 a.m. UTC | #4 
Hi AI,

> Umm...  What's the branchpoint for that series?
> Not the mainline - there we have i_blocksize() open-coded...

Sorry, I'm based on the latest branch of the erofs repository.

https://git.kernel.org/pub/scm/linux/kernel/git/xiang/erofs.git/log/?h=dev-test

I think I can resend based on mainline.

> Umm...  That actually asks for DIV_ROUND_UP(i_size_read_inode(), i_blocksize(inode))
> - compiler should bloody well be able to figure out that division by (1 << n)
> is shift down by n and it's easier to follow that way...

So it seems better to change to DIV_ROUND_UP(i_size_read_inode(), i_blocksize(inode))?

> And the fact that the value will be the same (i.e. that ->i_blkbits is never changed by ocfs2)
> is worth mentioning in commit message...

How about the following msg?

Use i_blockmask() to simplify code. BTW convert ocfs2_is_io_unaligned
to return bool type and the fact that the value will be the same
(i.e. that ->i_blkbits is never changed by ocfs2).



A small question, whether this series of changes will be merged
into each fs branch or all merged into vfs?

Thx,
Yangtao
Al Viro March 10, 2023, 4:05 a.m. UTC | #5 
On Fri, Mar 10, 2023 at 11:51:21AM +0800, Yangtao Li wrote:
> Hi AI,
> 
> > Umm...  What's the branchpoint for that series?
> > Not the mainline - there we have i_blocksize() open-coded...
> 
> Sorry, I'm based on the latest branch of the erofs repository.
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/xiang/erofs.git/log/?h=dev-test
> 
> I think I can resend based on mainline.
> 
> > Umm...  That actually asks for DIV_ROUND_UP(i_size_read_inode(), i_blocksize(inode))
> > - compiler should bloody well be able to figure out that division by (1 << n)
> > is shift down by n and it's easier to follow that way...
> 
> So it seems better to change to DIV_ROUND_UP(i_size_read_inode(), i_blocksize(inode))?
> 
> > And the fact that the value will be the same (i.e. that ->i_blkbits is never changed by ocfs2)
> > is worth mentioning in commit message...
> 
> How about the following msg?
> 
> Use i_blockmask() to simplify code. BTW convert ocfs2_is_io_unaligned
> to return bool type and the fact that the value will be the same
> (i.e. that ->i_blkbits is never changed by ocfs2).
> 
> 
> 
> A small question, whether this series of changes will be merged
> into each fs branch or all merged into vfs?

Depends.  The thing to avoid is conflicts between the trees and
convoluted commit graph.

In cases like that the usual approach is
	* put the helper into never-rebased branch - in vfs tree, in this
case; I've no real objections against the helper in question.
	* let other trees convert to the helper at leisure - merging
that never-rebased branch from vfs.git before they use the helper, of
course.  Or wait until the next cycle, for that matter...

I can pick the stuff in the areas that don't have active development,
but doing that for e.g. ext4 won't help anybody - it would only cause
headache for everyone involved down the road.  And I'd expect the gfs2
to be in the same situation...
李扬韬 March 10, 2023, 6:50 a.m. UTC | #6 
> I can pick the stuff in the areas that don't have active development.

Could you please consider helping to pick this
patch("ecryptfs: make splice write available again")?
ecryptfs seems to be unmaintained.

https://lore.kernel.org/lkml/20220831033505.23178-1-frank.li@vivo.com/

Thx,
Yangtao
diff mbox series

Patch

diff --git a/fs/erofs/data.c b/fs/erofs/data.c
index 7e8baf56faa5..e9d1869cd4b3 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 = i_blocksize(inode) - 1;
+			blksize_mask = i_blockmask(inode);
 
 		if ((iocb->ki_pos | iov_iter_count(to) |
 		     iov_iter_alignment(to)) & blksize_mask)