[PATCH v3 1/6] fs: add i_blockmask()

[PATCH v3 1/6] fs: add i_blockmask()

[Yangtao Li]

Introduce i_blockmask() to simplify code, which replace
(i_blocksize(node) - 1). Like done in commit
93407472a21b("fs: add i_blocksize()").

Signed-off-by: Yangtao Li <frank.li@vivo.com>
---
v3:
-none
v2:
-convert to i_blockmask()
 include/linux/fs.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index c85916e9f7db..17387d465b8b 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -711,6 +711,11 @@ static inline unsigned int i_blocksize(const struct inode *node)
 	return (1 << node->i_blkbits);
 }
 
+static inline unsigned int i_blockmask(const struct inode *node)
+{
+	return i_blocksize(node) - 1;
+}
+
 static inline int inode_unhashed(struct inode *inode)
 {
 	return hlist_unhashed(&inode->i_hash);
-- 
2.25.1

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

[Yangtao Li]

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(-)

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

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

[Al Viro]

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
> 

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

[Gao Xiang]

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

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

[Gao Xiang]

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

Re: erofs: convert to use i_blockmask()

[Yangtao Li]

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

Re: erofs: convert to use i_blockmask()

[Al Viro]

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

Re: erofs: convert to use i_blockmask()

[Yangtao Li]

> 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

[PATCH v3 3/6] gfs2: convert to use i_blockmask()

[Yangtao Li]

Use i_blockmask() to simplify code.

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

diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c
index eedf6926c652..1c6874b3851a 100644
--- a/fs/gfs2/bmap.c
+++ b/fs/gfs2/bmap.c
@@ -960,7 +960,7 @@ static struct folio *
 gfs2_iomap_get_folio(struct iomap_iter *iter, loff_t pos, unsigned len)
 {
 	struct inode *inode = iter->inode;
-	unsigned int blockmask = i_blocksize(inode) - 1;
+	unsigned int blockmask = i_blockmask(inode);
 	struct gfs2_sbd *sdp = GFS2_SB(inode);
 	unsigned int blocks;
 	struct folio *folio;
-- 
2.25.1

[PATCH v3 4/6] ext4: convert to use i_blockmask()

[Yangtao Li]

Use i_blockmask() to simplify code.

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

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index d251d705c276..eec36520e5e9 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -2218,7 +2218,7 @@ static int mpage_process_page_bufs(struct mpage_da_data *mpd,
 {
 	struct inode *inode = mpd->inode;
 	int err;
-	ext4_lblk_t blocks = (i_size_read(inode) + i_blocksize(inode) - 1)
+	ext4_lblk_t blocks = (i_size_read(inode) + i_blockmask(inode))
 							>> inode->i_blkbits;
 
 	if (ext4_verity_in_progress(inode))
-- 
2.25.1

Re: [PATCH v3 4/6] ext4: convert to use i_blockmask()

[Al Viro]

On Thu, Mar 09, 2023 at 11:21:25PM +0800, Yangtao Li wrote:
> Use i_blockmask() to simplify code.
> 
> Signed-off-by: Yangtao Li <frank.li@vivo.com>
> ---
> v3:
> -none
> v2:
> -convert to i_blockmask()
>  fs/ext4/inode.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index d251d705c276..eec36520e5e9 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -2218,7 +2218,7 @@ static int mpage_process_page_bufs(struct mpage_da_data *mpd,
>  {
>  	struct inode *inode = mpd->inode;
>  	int err;
> -	ext4_lblk_t blocks = (i_size_read(inode) + i_blocksize(inode) - 1)
> +	ext4_lblk_t blocks = (i_size_read(inode) + i_blockmask(inode))
>  							>> inode->i_blkbits;

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

Re: [PATCH v3 4/6] ext4: convert to use i_blockmask()

[Al Viro]

On Fri, Mar 10, 2023 at 03:19:40AM +0000, Al Viro wrote:
> On Thu, Mar 09, 2023 at 11:21:25PM +0800, Yangtao Li wrote:
> > Use i_blockmask() to simplify code.
> > 
> > Signed-off-by: Yangtao Li <frank.li@vivo.com>
> > ---
> > v3:
> > -none
> > v2:
> > -convert to i_blockmask()
> >  fs/ext4/inode.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> > index d251d705c276..eec36520e5e9 100644
> > --- a/fs/ext4/inode.c
> > +++ b/fs/ext4/inode.c
> > @@ -2218,7 +2218,7 @@ static int mpage_process_page_bufs(struct mpage_da_data *mpd,
> >  {
> >  	struct inode *inode = mpd->inode;
> >  	int err;
> > -	ext4_lblk_t blocks = (i_size_read(inode) + i_blocksize(inode) - 1)
> > +	ext4_lblk_t blocks = (i_size_read(inode) + i_blockmask(inode))
> >  							>> inode->i_blkbits;
> 
> 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...

BTW, here the fact that you are adding the mask is misleading - it's true,
but we are not using it as a mask - it's straight arithmetics.

One can do an equivalent code where it would've been used as a mask, but that
would be harder to read -
	(((i_size_read(inode) - 1) | i_blockmask(inode)) + 1) >> inode->i_blkbits
and I doubt anyone wants that kind of puzzles to stumble upon.

[PATCH v3 5/6] ocfs2: convert to use i_blockmask()

[Yangtao Li]

Use i_blockmask() to simplify code. BTW convert ocfs2_is_io_unaligned
to return bool type.

Signed-off-by: Yangtao Li <frank.li@vivo.com>
---
v3:
-none
 fs/ocfs2/file.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
index efb09de4343d..baefab3b12c9 100644
--- a/fs/ocfs2/file.c
+++ b/fs/ocfs2/file.c
@@ -2159,14 +2159,14 @@ int ocfs2_check_range_for_refcount(struct inode *inode, loff_t pos,
 	return ret;
 }
 
-static int ocfs2_is_io_unaligned(struct inode *inode, size_t count, loff_t pos)
+static bool ocfs2_is_io_unaligned(struct inode *inode, size_t count, loff_t pos)
 {
-	int blockmask = inode->i_sb->s_blocksize - 1;
+	int blockmask = i_blockmask(inode);
 	loff_t final_size = pos + count;
 
 	if ((pos & blockmask) || (final_size & blockmask))
-		return 1;
-	return 0;
+		return true;
+	return false;
 }
 
 static int ocfs2_inode_lock_for_extent_tree(struct inode *inode,
-- 
2.25.1

Re: [PATCH v3 5/6] ocfs2: convert to use i_blockmask()

[Al Viro]

On Thu, Mar 09, 2023 at 11:21:26PM +0800, Yangtao Li wrote:
> Use i_blockmask() to simplify code. BTW convert ocfs2_is_io_unaligned
> to return bool type.
> 
> Signed-off-by: Yangtao Li <frank.li@vivo.com>
> ---
> v3:
> -none
>  fs/ocfs2/file.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
> index efb09de4343d..baefab3b12c9 100644
> --- a/fs/ocfs2/file.c
> +++ b/fs/ocfs2/file.c
> @@ -2159,14 +2159,14 @@ int ocfs2_check_range_for_refcount(struct inode *inode, loff_t pos,
>  	return ret;
>  }
>  
> -static int ocfs2_is_io_unaligned(struct inode *inode, size_t count, loff_t pos)
> +static bool ocfs2_is_io_unaligned(struct inode *inode, size_t count, loff_t pos)
>  {
> -	int blockmask = inode->i_sb->s_blocksize - 1;
> +	int blockmask = i_blockmask(inode);
>  	loff_t final_size = pos + count;
>  
>  	if ((pos & blockmask) || (final_size & blockmask))
> -		return 1;
> -	return 0;
> +		return true;
> +	return false;
>  }

Ugh...
	return (pos | count) & blockmask;
surely?  Conversion to bool will take care of the rest.  Or you could make
that
	return ((pos | count) & blockmask) != 0;

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

[PATCH v3 6/6] fs/remap_range: convert to use i_blockmask()

[Yangtao Li]

Use i_blockmask() to simplify code.

Signed-off-by: Yangtao Li <frank.li@vivo.com>
---
 fs/remap_range.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/remap_range.c b/fs/remap_range.c
index 1331a890f2f2..7a524b620e7d 100644
--- a/fs/remap_range.c
+++ b/fs/remap_range.c
@@ -127,7 +127,7 @@ static int generic_remap_check_len(struct inode *inode_in,
 				   loff_t *len,
 				   unsigned int remap_flags)
 {
-	u64 blkmask = i_blocksize(inode_in) - 1;
+	u64 blkmask = i_blockmask(inode_in);
 	loff_t new_len = *len;
 
 	if ((*len & blkmask) == 0)
-- 
2.25.1