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