| Message ID | 20230309152127.41427-2-frank.li@vivo.com (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Headers | show |
| Series | [v3,1/6] fs: add i_blockmask() | expand |
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 >
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 >>
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 >>>
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...
> 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 --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)
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(-)