* Re: [PATCH v3 1/7] erofs: on-disk format should have explicitly assigned numbers [not found] ` <20190830033643.51019-1-gaoxiang25@huawei.com> @ 2019-08-30 15:25 ` Gao Xiang [not found] ` <20190830033643.51019-6-gaoxiang25@huawei.com> [not found] ` <20190830033643.51019-7-gaoxiang25@huawei.com> 2 siblings, 0 replies; 6+ messages in thread From: Gao Xiang @ 2019-08-30 15:25 UTC (permalink / raw) To: Chao Yu, Dan Carpenter, Christoph Hellwig, Joe Perches, Greg Kroah-Hartman, devel Cc: weidu.du, Miao Xie, linux-erofs, LKML On Fri, Aug 30, 2019 at 11:36:37AM +0800, Gao Xiang wrote: > As Christoph claimed [1], on-disk format should have > explicitly assigned numbers. I have to change it. > > [1] https://lore.kernel.org/r/20190829095954.GB20598@infradead.org/ > Reported-by: Christoph Hellwig <hch@infradead.org> > Reviewed-by: Chao Yu <yuchao0@huawei.com> > Signed-off-by: Gao Xiang <gaoxiang25@huawei.com> ...ignore this patchset. I will resend another incremental patchset to address what Christoph suggested yesterday... Thanks, Gao Xiang > --- > no change > > fs/erofs/erofs_fs.h | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/fs/erofs/erofs_fs.h b/fs/erofs/erofs_fs.h > index afa7d45ca958..2447ad4d0920 100644 > --- a/fs/erofs/erofs_fs.h > +++ b/fs/erofs/erofs_fs.h > @@ -52,10 +52,10 @@ struct erofs_super_block { > * 4~7 - reserved > */ > enum { > - EROFS_INODE_FLAT_PLAIN, > - EROFS_INODE_FLAT_COMPRESSION_LEGACY, > - EROFS_INODE_FLAT_INLINE, > - EROFS_INODE_FLAT_COMPRESSION, > + EROFS_INODE_FLAT_PLAIN = 0, > + EROFS_INODE_FLAT_COMPRESSION_LEGACY = 1, > + EROFS_INODE_FLAT_INLINE = 2, > + EROFS_INODE_FLAT_COMPRESSION = 3, > EROFS_INODE_LAYOUT_MAX > }; > > @@ -181,7 +181,7 @@ struct erofs_xattr_entry { > > /* available compression algorithm types */ > enum { > - Z_EROFS_COMPRESSION_LZ4, > + Z_EROFS_COMPRESSION_LZ4 = 0, > Z_EROFS_COMPRESSION_MAX > }; > > @@ -239,10 +239,10 @@ struct z_erofs_map_header { > * (di_advise could be 0, 1 or 2) > */ > enum { > - Z_EROFS_VLE_CLUSTER_TYPE_PLAIN, > - Z_EROFS_VLE_CLUSTER_TYPE_HEAD, > - Z_EROFS_VLE_CLUSTER_TYPE_NONHEAD, > - Z_EROFS_VLE_CLUSTER_TYPE_RESERVED, > + Z_EROFS_VLE_CLUSTER_TYPE_PLAIN = 0, > + Z_EROFS_VLE_CLUSTER_TYPE_HEAD = 1, > + Z_EROFS_VLE_CLUSTER_TYPE_NONHEAD = 2, > + Z_EROFS_VLE_CLUSTER_TYPE_RESERVED = 3, > Z_EROFS_VLE_CLUSTER_TYPE_MAX > }; > > -- > 2.17.1 > _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
[parent not found: <20190830033643.51019-6-gaoxiang25@huawei.com>]
* Re: [PATCH v3 6/7] erofs: remove all likely/unlikely annotations [not found] ` <20190830033643.51019-6-gaoxiang25@huawei.com> @ 2019-08-30 15:46 ` Christoph Hellwig 2019-08-30 16:04 ` Gao Xiang 0 siblings, 1 reply; 6+ messages in thread From: Christoph Hellwig @ 2019-08-30 15:46 UTC (permalink / raw) To: Gao Xiang Cc: devel, Chao Yu, Greg Kroah-Hartman, Miao Xie, Chao Yu, LKML, Christoph Hellwig, weidu.du, Fang Wei, Joe Perches, linux-erofs, Dan Carpenter On Fri, Aug 30, 2019 at 11:36:42AM +0800, Gao Xiang wrote: > As Dan Carpenter suggested [1], I have to remove > all erofs likely/unlikely annotations. Do you have to remove all of them, or just those where you don't have a particularly good reason why you think in this particular case they might actually matter? _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 6/7] erofs: remove all likely/unlikely annotations 2019-08-30 15:46 ` [PATCH v3 6/7] erofs: remove all likely/unlikely annotations Christoph Hellwig @ 2019-08-30 16:04 ` Gao Xiang 2019-08-31 10:57 ` Dan Carpenter 0 siblings, 1 reply; 6+ messages in thread From: Gao Xiang @ 2019-08-30 16:04 UTC (permalink / raw) To: Christoph Hellwig Cc: devel, Chao Yu, Greg Kroah-Hartman, Miao Xie, Chao Yu, LKML, weidu.du, Fang Wei, Joe Perches, linux-erofs, Dan Carpenter Hi Christoph, On Fri, Aug 30, 2019 at 08:46:50AM -0700, Christoph Hellwig wrote: > On Fri, Aug 30, 2019 at 11:36:42AM +0800, Gao Xiang wrote: > > As Dan Carpenter suggested [1], I have to remove > > all erofs likely/unlikely annotations. > > Do you have to remove all of them, or just those where you don't have > a particularly good reason why you think in this particular case they > might actually matter? I just added unlikely/likely for all erofs error handling paths or rare happened cases at first... (That is all in my thought...) I don't have some benchmark data for each unlikely/likely case (and I have no idea "is that worth to take time to benchmark rather than do another more useful stuffs"), so..I have to kill them all... Thanks, Gao Xiang _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 6/7] erofs: remove all likely/unlikely annotations 2019-08-30 16:04 ` Gao Xiang @ 2019-08-31 10:57 ` Dan Carpenter 0 siblings, 0 replies; 6+ messages in thread From: Dan Carpenter @ 2019-08-31 10:57 UTC (permalink / raw) To: Gao Xiang Cc: devel, Chao Yu, Greg Kroah-Hartman, Miao Xie, Chao Yu, LKML, Christoph Hellwig, weidu.du, Fang Wei, Joe Perches, linux-erofs On Sat, Aug 31, 2019 at 12:04:20AM +0800, Gao Xiang wrote: > I don't have some benchmark data for each unlikely/likely case (and I have > no idea "is that worth to take time to benchmark rather than do another more > useful stuffs"), so..I have to kill them all... We don't really require benchmarks, just that a reasonable person would think it might make a difference. regards, dan carpenter _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
[parent not found: <20190830033643.51019-7-gaoxiang25@huawei.com>]
* Re: [PATCH v3 7/7] erofs: redundant assignment in __erofs_get_meta_page() [not found] ` <20190830033643.51019-7-gaoxiang25@huawei.com> @ 2019-08-30 16:28 ` Christoph Hellwig 2019-08-30 16:48 ` Gao Xiang 0 siblings, 1 reply; 6+ messages in thread From: Christoph Hellwig @ 2019-08-30 16:28 UTC (permalink / raw) To: Gao Xiang Cc: devel, Chao Yu, Greg Kroah-Hartman, Miao Xie, Chao Yu, LKML, Christoph Hellwig, weidu.du, Fang Wei, Joe Perches, linux-erofs, Dan Carpenter > - err = bio_add_page(bio, page, PAGE_SIZE, 0); > - if (err != PAGE_SIZE) { > + if (bio_add_page(bio, page, PAGE_SIZE, 0) != PAGE_SIZE) { > err = -EFAULT; > goto err_out; > } This patch looks like an improvement. But looking at that whole area just makes me cringe. Why is there __erofs_get_meta_page with the two weird booleans instead of a single erofs_get_meta_page that gets and gfp_t for additional flags and an unsigned int for additional bio op flags. Why do need ioprio support to start with? Seeing that in a new fs look kinda odd. Do you have benchmarks that show the difference? That function then calls erofs_grab_bio, which tries to handle a bio_alloc failure, except that the function will not actually fail due the mempool backing it. It also seems like and awfully huge function to inline. Why is there __submit_bio which really just obsfucates what is going on? Also why is __submit_bio using bio_set_op_attrs instead of opencode it as the comment right next to it asks you to? Also I really don't understand why you can't just use read_cache_page or even read_cache_page_gfp instead of __erofs_get_meta_page. That function is a whole lot of duplication of functionality shared by a lot of other file systems. _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 7/7] erofs: redundant assignment in __erofs_get_meta_page() 2019-08-30 16:28 ` [PATCH v3 7/7] erofs: redundant assignment in __erofs_get_meta_page() Christoph Hellwig @ 2019-08-30 16:48 ` Gao Xiang 0 siblings, 0 replies; 6+ messages in thread From: Gao Xiang @ 2019-08-30 16:48 UTC (permalink / raw) To: Christoph Hellwig Cc: devel, Chao Yu, Greg Kroah-Hartman, Miao Xie, Chao Yu, LKML, weidu.du, Fang Wei, Joe Perches, linux-erofs, Dan Carpenter Hi Christoph, On Fri, Aug 30, 2019 at 09:28:12AM -0700, Christoph Hellwig wrote: > > - err = bio_add_page(bio, page, PAGE_SIZE, 0); > > - if (err != PAGE_SIZE) { > > + if (bio_add_page(bio, page, PAGE_SIZE, 0) != PAGE_SIZE) { > > err = -EFAULT; > > goto err_out; > > } > > This patch looks like an improvement. But looking at that whole > area just makes me cringe. OK, I agree with you, I will improve it or just kill them all with new iomap approach after it supports tail-end packing inline. > > Why is there __erofs_get_meta_page with the two weird booleans instead > of a single erofs_get_meta_page that gets and gfp_t for additional > flags and an unsigned int for additional bio op flags. I agree with you. Thanks for your suggestion. > > Why do need ioprio support to start with? Seeing that in a new > fs look kinda odd. Do you have benchmarks that show the difference? I don't have some benchmark for all of these, can I just set REQ_PRIO for all metadata? is that reasonable? Could you kindly give some suggestion on this? > > That function then calls erofs_grab_bio, which tries to handle a > bio_alloc failure, except that the function will not actually fail > due the mempool backing it. It also seems like and awfully > huge function to inline. OK, I will simplify it. Thanks for your suggestion. > > Why is there __submit_bio which really just obsfucates what is > going on? Also why is __submit_bio using bio_set_op_attrs instead > of opencode it as the comment right next to it asks you to? Originally, mainly due to backport consideration since some of our smartphones use 3.x kernel as well... > > Also I really don't understand why you can't just use read_cache_page > or even read_cache_page_gfp instead of __erofs_get_meta_page. > That function is a whole lot of duplication of functionality shared > by a lot of other file systems. OK, I have to admit, that code was originally just copied from f2fs with some modification (maybe it's not a good example for us). Thanks, Gao Xiang _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-08-31 10:58 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <20190830032006.GA20217@architecture4> [not found] ` <20190830033643.51019-1-gaoxiang25@huawei.com> 2019-08-30 15:25 ` [PATCH v3 1/7] erofs: on-disk format should have explicitly assigned numbers Gao Xiang [not found] ` <20190830033643.51019-6-gaoxiang25@huawei.com> 2019-08-30 15:46 ` [PATCH v3 6/7] erofs: remove all likely/unlikely annotations Christoph Hellwig 2019-08-30 16:04 ` Gao Xiang 2019-08-31 10:57 ` Dan Carpenter [not found] ` <20190830033643.51019-7-gaoxiang25@huawei.com> 2019-08-30 16:28 ` [PATCH v3 7/7] erofs: redundant assignment in __erofs_get_meta_page() Christoph Hellwig 2019-08-30 16:48 ` Gao Xiang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).