Re: [Cluster-devel] [PATCH v4 2/5] erofs: convert to use i_blockmask()
On 2023/3/10 14:15, Yangtao Li wrote: Hi Gao Xiang, Please help drop this one since we'd like to use it until i_blockmask() lands to upstream. I'm OK. Not sure if I need to resend v5? Thanks, your patch looks fine to me. The main reasons are that 1) active cross tree development on cleanup stuffs; 2) we'd like to add subpage block support for the next cycle, and they seem somewhat convolved... So I will apply your patch when i_blockmask() is landed upstream then. Thanks, Gao Xiang Thx, Yangtao
Re: [Cluster-devel] [PATCH v4 2/5] erofs: convert to use i_blockmask()
Hi Yangtao, On 2023/3/10 13:48, Yangtao Li wrote: Use i_blockmask() to simplify code. Signed-off-by: Yangtao Li Please help drop this one since we'd like to use it until i_blockmask() lands to upstream. 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 e16545849ea7..d394102ef9de 100644 --- a/fs/erofs/data.c +++ b/fs/erofs/data.c @@ -376,7 +376,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_blockmask(inode); if ((iocb->ki_pos | iov_iter_count(to) | iov_iter_alignment(to)) & blksize_mask)
Re: [Cluster-devel] [PATCH v3 2/6] erofs: convert to use i_blockmask()
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-e54861fe4...@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 --- 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: [Cluster-devel] [PATCH v3 2/6] erofs: convert to use i_blockmask()
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-e54861fe4...@kernel.org/T/#t Sorry this link: https://lore.kernel.org/r/0261de31-e98b-85cd-80de-96af5a76e...@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 --- 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: [Cluster-devel] [5.15 REGRESSION v2] iomap: Fix inline extent handling in iomap_readpage
On Tue, Nov 16, 2021 at 09:33:30PM -0800, Darrick J. Wong wrote:
> On Thu, Nov 11, 2021 at 05:17:14PM +0100, Andreas Gruenbacher wrote:
> > Before commit 740499c78408 ("iomap: fix the iomap_readpage_actor return
> > value for inline data"), when hitting an IOMAP_INLINE extent,
> > iomap_readpage_actor would report having read the entire page. Since
> > then, it only reports having read the inline data (iomap->length).
> >
> > This will force iomap_readpage into another iteration, and the
> > filesystem will report an unaligned hole after the IOMAP_INLINE extent.
> > But iomap_readpage_actor (now iomap_readpage_iter) isn't prepared to
> > deal with unaligned extents, it will get things wrong on filesystems
> > with a block size smaller than the page size, and we'll eventually run
> > into the following warning in iomap_iter_advance:
> >
> > WARN_ON_ONCE(iter->processed > iomap_length(iter));
> >
> > Fix that by changing iomap_readpage_iter to return 0 when hitting an
> > inline extent; this will cause iomap_iter to stop immediately.
>
> I guess this means that we also only support having inline data that
> ends at EOF? IIRC this is true for the three(?) filesystems that have
> expressed any interest in inline data: yours, ext4, and erofs?
For erofs, confirmed. (also yes in the long run...)
Thanks,
Gao Xiang
Re: [Cluster-devel] [PATCH v11 19/25] erofs: Convert compressed files from readpages to readahead
Hi Andrew,
On Mon, Apr 20, 2020 at 10:42:10PM -0700, Andrew Morton wrote:
> On Tue, 14 Apr 2020 08:02:27 -0700 Matthew Wilcox wrote:
>
> >
> > Use the new readahead operation in erofs.
> >
>
> Well this is exciting.
>
> fs/erofs/data.c: In function erofs_raw_access_readahead:
> fs/erofs/data.c:149:18: warning: last_block may be used uninitialized in this
> function [-Wmaybe-uninitialized]
> *last_block + 1 != current_block) {
>
> It seems to be a preexisting bug, which your patch prompted gcc-7.2.0
> to notice.
>
> erofs_read_raw_page() goes in and uses *last_block, but neither of its
> callers has initialized it. Could the erofs maintainers please take a
> look?
simply because last_block doesn't need to be initialized at first,
because bio == NULL in the begining anyway. I believe this is a gcc
false warning because some gcc versions raised some before (many gccs
don't, including my current gcc (Debian 8.3.0-6) 8.3.0).
in detail,
146 /* note that for readpage case, bio also equals to NULL */
147 if (bio &&
148 /* not continuous */
149 *last_block + 1 != current_block) {
150 submit_bio_retry:
151 submit_bio(bio);
152 bio = NULL;
153 }
bio will be NULL and will bypass the next condition at first.
after that,
155 if (!bio) {
...
221 bio = bio_alloc(GFP_NOIO, nblocks);
...
}
...
230 err = bio_add_page(bio, page, PAGE_SIZE, 0);
231 /* out of the extent or bio is full */
232 if (err < PAGE_SIZE)
233 goto submit_bio_retry;
234
235 *last_block = current_block;
so bio != NULL, and last_block will be assigned then as well.
Thanks,
Gao Xiang
Re: [Cluster-devel] [PATCH v6 11/16] erofs: Convert compressed files from readpages to readahead
On Mon, Feb 17, 2020 at 10:46:00AM -0800, Matthew Wilcox wrote:
> From: "Matthew Wilcox (Oracle)"
>
> Use the new readahead operation in erofs.
>
> Signed-off-by: Matthew Wilcox (Oracle)
It looks good to me, although some further optimization exists
but we could make a straight-forward transform first, and
I haven't tested the whole series for now...
Will test it later.
Acked-by: Gao Xiang
Thanks,
Gao Xiang
> ---
> fs/erofs/zdata.c | 29 +
> 1 file changed, 9 insertions(+), 20 deletions(-)
>
> diff --git a/fs/erofs/zdata.c b/fs/erofs/zdata.c
> index 17f45fcb8c5c..7c02015d501d 100644
> --- a/fs/erofs/zdata.c
> +++ b/fs/erofs/zdata.c
> @@ -1303,28 +1303,23 @@ static bool should_decompress_synchronously(struct
> erofs_sb_info *sbi,
> return nr <= sbi->max_sync_decompress_pages;
> }
>
> -static int z_erofs_readpages(struct file *filp, struct address_space
> *mapping,
> - struct list_head *pages, unsigned int nr_pages)
> +static void z_erofs_readahead(struct readahead_control *rac)
> {
> - struct inode *const inode = mapping->host;
> + struct inode *const inode = rac->mapping->host;
> struct erofs_sb_info *const sbi = EROFS_I_SB(inode);
>
> - bool sync = should_decompress_synchronously(sbi, nr_pages);
> + bool sync = should_decompress_synchronously(sbi, readahead_count(rac));
> struct z_erofs_decompress_frontend f = DECOMPRESS_FRONTEND_INIT(inode);
> - gfp_t gfp = mapping_gfp_constraint(mapping, GFP_KERNEL);
> - struct page *head = NULL;
> + struct page *page, *head = NULL;
> LIST_HEAD(pagepool);
>
> - trace_erofs_readpages(mapping->host, lru_to_page(pages)->index,
> - nr_pages, false);
> + trace_erofs_readpages(inode, readahead_index(rac),
> + readahead_count(rac), false);
>
> - f.headoffset = (erofs_off_t)lru_to_page(pages)->index << PAGE_SHIFT;
> -
> - for (; nr_pages; --nr_pages) {
> - struct page *page = lru_to_page(pages);
> + f.headoffset = readahead_offset(rac);
>
> + readahead_for_each(rac, page) {
> prefetchw(&page->flags);
> - list_del(&page->lru);
>
> /*
>* A pure asynchronous readahead is indicated if
> @@ -1333,11 +1328,6 @@ static int z_erofs_readpages(struct file *filp, struct
> address_space *mapping,
>*/
> sync &= !(PageReadahead(page) && !head);
>
> - if (add_to_page_cache_lru(page, mapping, page->index, gfp)) {
> - list_add(&page->lru, &pagepool);
> - continue;
> - }
> -
> set_page_private(page, (unsigned long)head);
> head = page;
> }
> @@ -1366,11 +1356,10 @@ static int z_erofs_readpages(struct file *filp,
> struct address_space *mapping,
>
> /* clean up the remaining free pages */
> put_pages_list(&pagepool);
> - return 0;
> }
>
> const struct address_space_operations z_erofs_aops = {
> .readpage = z_erofs_readpage,
> - .readpages = z_erofs_readpages,
> + .readahead = z_erofs_readahead,
> };
>
> --
> 2.25.0
>
>
Re: [Cluster-devel] [PATCH v6 12/19] erofs: Convert uncompressed files from readpages to readahead
On Mon, Feb 17, 2020 at 10:46:01AM -0800, Matthew Wilcox wrote:
> From: "Matthew Wilcox (Oracle)"
>
> Use the new readahead operation in erofs
>
> Signed-off-by: Matthew Wilcox (Oracle)
> ---
It looks good to me, and will test it later as well..
Acked-by: Gao Xiang
Thanks,
Gao Xiang
> fs/erofs/data.c | 39 +---
> fs/erofs/zdata.c | 2 +-
> include/trace/events/erofs.h | 6 +++---
> 3 files changed, 18 insertions(+), 29 deletions(-)
>
> diff --git a/fs/erofs/data.c b/fs/erofs/data.c
> index fc3a8d8064f8..82ebcee9d178 100644
> --- a/fs/erofs/data.c
> +++ b/fs/erofs/data.c
> @@ -280,47 +280,36 @@ static int erofs_raw_access_readpage(struct file *file,
> struct page *page)
> return 0;
> }
>
> -static int erofs_raw_access_readpages(struct file *filp,
> - struct address_space *mapping,
> - struct list_head *pages,
> - unsigned int nr_pages)
> +static void erofs_raw_access_readahead(struct readahead_control *rac)
> {
> erofs_off_t last_block;
> struct bio *bio = NULL;
> - gfp_t gfp = readahead_gfp_mask(mapping);
> - struct page *page = list_last_entry(pages, struct page, lru);
> -
> - trace_erofs_readpages(mapping->host, page, nr_pages, true);
> + struct page *page;
>
> - for (; nr_pages; --nr_pages) {
> - page = list_entry(pages->prev, struct page, lru);
> + trace_erofs_readpages(rac->mapping->host, readahead_index(rac),
> + readahead_count(rac), true);
>
> + readahead_for_each(rac, page) {
> prefetchw(&page->flags);
> - list_del(&page->lru);
>
> - if (!add_to_page_cache_lru(page, mapping, page->index, gfp)) {
> - bio = erofs_read_raw_page(bio, mapping, page,
> - &last_block, nr_pages, true);
> + bio = erofs_read_raw_page(bio, rac->mapping, page, &last_block,
> + readahead_count(rac), true);
>
> - /* all the page errors are ignored when readahead */
> - if (IS_ERR(bio)) {
> - pr_err("%s, readahead error at page %lu of nid
> %llu\n",
> -__func__, page->index,
> -EROFS_I(mapping->host)->nid);
> + /* all the page errors are ignored when readahead */
> + if (IS_ERR(bio)) {
> + pr_err("%s, readahead error at page %lu of nid %llu\n",
> +__func__, page->index,
> +EROFS_I(rac->mapping->host)->nid);
>
> - bio = NULL;
> - }
> + bio = NULL;
> }
>
> - /* pages could still be locked */
> put_page(page);
> }
> - DBG_BUGON(!list_empty(pages));
>
> /* the rare case (end in gaps) */
> if (bio)
> submit_bio(bio);
> - return 0;
> }
>
> static int erofs_get_block(struct inode *inode, sector_t iblock,
> @@ -358,7 +347,7 @@ static sector_t erofs_bmap(struct address_space *mapping,
> sector_t block)
> /* for uncompressed (aligned) files and raw access for other files */
> const struct address_space_operations erofs_raw_access_aops = {
> .readpage = erofs_raw_access_readpage,
> - .readpages = erofs_raw_access_readpages,
> + .readahead = erofs_raw_access_readahead,
> .bmap = erofs_bmap,
> };
>
> diff --git a/fs/erofs/zdata.c b/fs/erofs/zdata.c
> index 80e47f07d946..17f45fcb8c5c 100644
> --- a/fs/erofs/zdata.c
> +++ b/fs/erofs/zdata.c
> @@ -1315,7 +1315,7 @@ static int z_erofs_readpages(struct file *filp, struct
> address_space *mapping,
> struct page *head = NULL;
> LIST_HEAD(pagepool);
>
> - trace_erofs_readpages(mapping->host, lru_to_page(pages),
> + trace_erofs_readpages(mapping->host, lru_to_page(pages)->index,
> nr_pages, false);
>
> f.headoffset = (erofs_off_t)lru_to_page(pages)->index << PAGE_SHIFT;
> diff --git a/include/trace/events/erofs.h b/include/trace/events/erofs.h
> index 27f5caa6299a..bf9806fd1306 100644
> --- a/include/trace/events/erofs.h
> +++ b/include/trace/events/erofs.h
> @@ -113,10 +113,10 @@ TRACE_EVENT(erofs_readpage,
>
> TRACE_EVENT(erofs_readpages,
>
> - TP_PROTO(struct inode *inod
Re: [Cluster-devel] [PATCH V10 03/19] block: use bio_for_each_bvec() to compute multi-page bvec count
On 2018/11/16 17:19, Christoph Hellwig wrote: > On Thu, Nov 15, 2018 at 02:18:47PM -0800, Omar Sandoval wrote: >> My only reason to prefer unsigned int is consistency. unsigned int is >> much more common in the kernel: >> >> $ ag --cc -s 'unsigned\s+int' | wc -l >> 129632 >> $ ag --cc -s 'unsigned\s+(?!char|short|int|long)' | wc -l >> 22435 >> >> checkpatch also warns on plain unsigned. > > Talk about chicken and egg. unsigned is perfectly valid C, and being > shorter often helps being more readable. checkpath is as so often > wrongly opinionated.. > sigh...I personally tend to use "unsigned" instead of "unsigned int" as well, but checkpatch.pl also suggests erofs to use "unsigned int" :-( Thanks, Gao Xiang
