Ocfs2 devel - Mar 2023 - [PATCH v3 2/6] erofs: convert to use i_blockmask()

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 at 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
>

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 at
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 at 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
>>

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

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...