Re: [PATCH v2] staging: erofs: use explicit unsigned int type
struct erofs_inode_v1 *v1 = data; > - const unsigned advise = le16_to_cpu(v1->i_advise); > + const unsigned int advise = le16_to_cpu(v1->i_advise); > > vi->data_mapping_mode = __inode_data_mapping(advise); > > @@ -112,7 +112,7 @@ static int read_inode(struct inode *inode, void *data) > * try_lock since it takes no much overhead and > * will success immediately. > */ > -static int fill_inline_data(struct inode *inode, void *data, unsigned m_pofs) > +static int fill_inline_data(struct inode *inode, void *data, unsigned int > m_pofs) WARNING: line over 80 characters #124: FILE: drivers/staging/erofs/inode.c:115: +static int fill_inline_data(struct inode *inode, void *data, unsigned int m_pofs) Thanks, Gao Xiang ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] MAINTAINERS: add tree location for staging/erofs
On 2018/9/10 2:34, Thomas Weißschuh wrote: > Hi Chao, hi Gao, > > On Sun, 2018-09-09T23:16+0800, Chao Yu wrote: >> Hi Thomas, >> >> On 2018/9/8 11:28, Gao Xiang wrote: >>> Hi Thomas, >>> >>> Thanks for your new patch. >>> >>> It seems that this patch also introduces 2 new 'WARNING: line over 80 >>> characters', >>> could you please fix it in the patch? > > @Chao,Gao: For some reason there seems to be a problem when receiving mails > from you. The one I received now was the first one from you that reached me > directly. Before I only got your messages in the quotes from Dan and now in > the > quotes from the mail I am responding to. > (I will try to investigate the reason for this) > > Sorry for my slow responses before. > >> It will be better to fix that. > > A new patch will shortly follow. > >> Some reviewed cleanup patches are merged before this patch, then I failed to >> add >> this one, so could you rebase on top of erofs dev branch in below link: >> >> https://git.kernel.org/pub/scm/linux/kernel/git/chao/linux.git/log/?h=erofs > > I was not aware of this tree and worked off of staging / next. > A patch is attached to this message that adds the tree to the MAINTAINERS > file. Hi Chao, I think this tree has some PREVIEW patches which preview in linux-erofs mailing list only and doesn't send to staging mailing list and LKML, https://git.kernel.org/pub/scm/linux/kernel/git/chao/linux.git/log/?h=erofs so erofs tree is actually Greg's staging tree. > > -- >8 -- > > Currently this location is not documented. > > Signed-off-by: Thomas Weißschuh > --- > MAINTAINERS | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/MAINTAINERS b/MAINTAINERS > index a5b256b25905..9087e0b74821 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -13747,6 +13747,7 @@ F:drivers/staging/comedi/ > STAGING - EROFS FILE SYSTEM > M: Gao Xiang > M: Chao Yu > +T: git git://git.kernel.org/pub/scm/linux/kernel/git/chao/linux.git erofs Hi Thomas, nope, the erofs tree is actually the same as staging tree, so any patch should be based on Greg's upstream tree. Thanks, Gao Xiang > L: linux-er...@lists.ozlabs.org > S: Maintained > F: drivers/staging/erofs/ > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/2 dhowells/mount-api] staging: erofs: rename superblock flags (MS_xyz -> SB_xyz)
Hi Greg,
On 2018/9/10 16:13, Greg Kroah-Hartman wrote:
> On Thu, Sep 06, 2018 at 06:37:37PM +0800, Gao Xiang wrote:
>>
>>
>> On 2018/9/6 18:08, David Howells wrote:
>>> Gao Xiang wrote:
>>>
>>>> This patch follows commit 1751e8a6cb93 ("Rename superblock
>>>> flags (MS_xyz -> SB_xyz)") and after commit ("vfs: Suppress
>>>> MS_* flag defs within the kernel unless explicitly enabled"),
>>>> there is no MS_RDONLY and MS_NOATIME at all.
>>>>
>>>> Reported-by: Stephen Rothwell
>>>> Reviewed-by: Chao Yu
>>>> Signed-off-by: Gao Xiang
>>> I recommend pushing this one to Linus now. It's trivial enough.
>>
>> Hi David,
>> I personally think that is fine, but I'd like to get Greg's idea and
>> agreement, too...
>>
>>>
>>> Reviewed-by: David Howells
>>
>> Hi Greg,
>>
>> As David suggested above, could you please help merge this patch to Linus?
>> It's trivial enough (since the commit 1751e8a6cb93) and MS_xyz will be
>> removed
>> in the new mount apis patchset...
>
> Now applied to my local tree, thanks.
>
Thanks for applying. :)
Thanks,
Gao Xiang
> greg k-h
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] erofs: surround fault_injection ralted option parsing using CONFIG_EROFS_FAULT_INJECTION
The fault injection code was taken from f2fs.
and I saw that Chengguang Xu submits a similar patch to f2fs-devel.
https://sourceforge.net/p/linux-f2fs/mailman/message/36412022/
In the future, it could use the common #include
like other modules I guess?
mm/slab_common.c
1555 int should_failslab(struct kmem_cache *s, gfp_t gfpflags)
1556 {
1557 if (__should_failslab(s, gfpflags))
1558 return -ENOMEM;
1559 return 0;
1560 }
1561 ALLOW_ERROR_INJECTION(should_failslab, ERRNO);
mm/failslab.c
17 bool __should_failslab(struct kmem_cache *s, gfp_t gfpflags)
18 {
19 /* No fault-injection for bootstrap cache */
20 if (unlikely(s == kmem_cache))
21 return false;
22
23 if (gfpflags & __GFP_NOFAIL)
24 return false;
25
26 if (failslab.ignore_gfp_reclaim && (gfpflags & __GFP_RECLAIM))
27 return false;
28
29 if (failslab.cache_filter && !(s->flags & SLAB_FAILSLAB))
30 return false;
31
32 return should_fail(&failslab.attr, s->object_size);
33 }
Thanks,
Gao Xiang
>
> thanks,
>
> greg k-h
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] MAINTAINERS: add tree location for staging/erofs
Hi Thomas, On 2018/9/10 23:59, Chao Yu wrote: > On 2018/9/10 11:56, Gao Xiang wrote: >> >> >> On 2018/9/10 2:34, Thomas Weißschuh wrote: >>> Hi Chao, hi Gao, >>> >>> On Sun, 2018-09-09T23:16+0800, Chao Yu wrote: >>>> Hi Thomas, >>>> >>>> On 2018/9/8 11:28, Gao Xiang wrote: >>>>> Hi Thomas, >>>>> >>>>> Thanks for your new patch. >>>>> >>>>> It seems that this patch also introduces 2 new 'WARNING: line over 80 >>>>> characters', >>>>> could you please fix it in the patch? >>> >>> @Chao,Gao: For some reason there seems to be a problem when receiving mails >>> from you. The one I received now was the first one from you that reached me >>> directly. Before I only got your messages in the quotes from Dan and now in >>> the >>> quotes from the mail I am responding to. >>> (I will try to investigate the reason for this) >>> >>> Sorry for my slow responses before. >>> >>>> It will be better to fix that. >>> >>> A new patch will shortly follow. >>> >>>> Some reviewed cleanup patches are merged before this patch, then I failed >>>> to add >>>> this one, so could you rebase on top of erofs dev branch in below link: >>>> >>>> https://git.kernel.org/pub/scm/linux/kernel/git/chao/linux.git/log/?h=erofs >>> >>> I was not aware of this tree and worked off of staging / next. >>> A patch is attached to this message that adds the tree to the MAINTAINERS >>> file. >> >> Hi Chao, >> >> I think this tree has some PREVIEW patches which preview in linux-erofs >> mailing list only and >> doesn't send to staging mailing list and LKML, >> >> https://git.kernel.org/pub/scm/linux/kernel/git/chao/linux.git/log/?h=erofs >> >> so erofs tree is actually Greg's staging tree. > > Thomas, > > I confirmed that erofs git repository for linux upstream is Greg's staging > tree. > > Let me explain, in order to avoid sending buggy or preview patch, Xiang and me > plan to review patches in erofs mailing list first, and then cache reviewed > patches in my git tree before sending them to Greg and staging mailing list. > > Based on that, I'm trying to serialize all erofs patches, expecting that can > help those patches sent to staging mailing list can be merged by Greg with > lesser conflict. But I made a mistake that my erofs branch has merged some > pending patches, result in failing to merge yours, that mislead me to ask you > to > rebase the code, sorry about that. As Chao's said, we keep on fetching the latest Greg's staging tree and develop , preview, and test new erofs features and bugfix based on it. For the cleanup patches, it could be better directly based on Greg's upstream staging tree. We will then rebase our developping code on your work. :) > > Now I can confirm that your v2 patch can apply on Greg's staging-next, so > fixing > warning reported by checkpatch.pl on your v2 patch is enough. :) Yes, we are sorry about that. could you please send v4 patch which follows Greg's and Chao's suggestions? Thanks, Gao Xiang > > Thanks, > >> >>> >>> -- >8 -- >>> >>> Currently this location is not documented. >>> >>> Signed-off-by: Thomas Weißschuh >>> --- >>> MAINTAINERS | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/MAINTAINERS b/MAINTAINERS >>> index a5b256b25905..9087e0b74821 100644 >>> --- a/MAINTAINERS >>> +++ b/MAINTAINERS >>> @@ -13747,6 +13747,7 @@ F: drivers/staging/comedi/ >>> STAGING - EROFS FILE SYSTEM >>> M: Gao Xiang >>> M: Chao Yu >>> +T: git git://git.kernel.org/pub/scm/linux/kernel/git/chao/linux.git erofs >> >> Hi Thomas, >> nope, the erofs tree is actually the same as staging tree, so any patch >> should be based >> on Greg's upstream tree. >> >> Thanks, >> Gao Xiang >> >>> L: linux-er...@lists.ozlabs.org >>> S: Maintained >>> F: drivers/staging/erofs/ >>> ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] erofs: option validation in remount
Hi Chengguang,
Thanks for your patch.
The patch title should be "staging: erofs: " since erofs is still in staging.
The same as your previous patch "erofs: surround fault_injection ralted option
parsing using CONFIG_EROFS_FAULT_INJECTION".
On 2018/9/11 18:51, Chengguang Xu wrote:
> Add option validation in remount. After this patch, remount
> can change recognized options, and for unknown options remount
> will fail and report error.
>
> Signed-off-by: Chengguang Xu
> ---
> drivers/staging/erofs/super.c | 17 +
> 1 file changed, 17 insertions(+)
>
> diff --git a/drivers/staging/erofs/super.c b/drivers/staging/erofs/super.c
> index 1aec509c805f..8bab077381ad 100644
> --- a/drivers/staging/erofs/super.c
> +++ b/drivers/staging/erofs/super.c
> @@ -625,10 +625,27 @@ static int erofs_show_options(struct seq_file *seq,
> struct dentry *root)
>
> static int erofs_remount(struct super_block *sb, int *flags, char *data)
> {
> + struct erofs_sb_info *sbi = EROFS_SB(sb);
> + struct erofs_fault_info *ffi = &sbi->fault_info;
> + unsigned int orig_mount_opt = sbi->mount_opt;
> + unsigned int orig_inject_rate = ffi->inject_rate;
> + int err;
> +
> BUG_ON(!sb_rdonly(sb));
>
> + err = parse_options(sb, data);
> + if (err)
> + goto out;
> +
> *flags |= MS_RDONLY;
> return 0;
> +
> +out:
> + if (ffi->inject_rate != orig_inject_rate)
> + erofs_build_fault_attr(sbi, orig_inject_rate);
Currently should be with "#ifdef CONFIG_EROFS_FAULT_INJECTION"?
and have you tried to compile without EROFS_FAULT_INJECTION?
Thanks,
Gao Xiang
> + sbi->mount_opt = orig_mount_opt;
> +
> + return err;
> }
>
> const struct super_operations erofs_sops = {
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [staging:staging-next 118/220] drivers/staging/erofs/unzip_vle.c:1003:1-7: preceding lock on line 839 (fwd)
Hi Julia,
There were lkp logs reporting that before, but I have no idea why it is
reported.
z_pagemap_global_lock is taken iff (pages = z_pagemap_global), and it is
unlocked at
> 3883a79a Gao Xiang 2018-07-26 987 if (pages == z_pagemap_global)
> 3883a79a Gao Xiang 2018-07-26 988
> mutex_unlock(&z_pagemap_global_lock);
It is designed on purpose for now. actually erofs is actually running on our
commerical
products but I haven't observed lock leaking. h...
If I am wrong, please point out thanks...
Thanks,
Gao Xiang
On 2018/9/11 21:40, Julia Lawall wrote:
> I don't know if there is a problem here. Please check the lines mentiond
> below, ie lines 839 and 1003.
>
> julia
>
> -- Forwarded message --
> Date: Tue, 11 Sep 2018 17:17:15 +0800
> From: kbuild test robot
> To: kbu...@01.org
> Cc: Julia Lawall
> Subject: [staging:staging-next 118/220]
> drivers/staging/erofs/unzip_vle.c:1003:1-7: preceding lock on line 839
>
> CC: kbuild-...@01.org
> CC: de...@driverdev.osuosl.org
> TO: Gao Xiang
> CC: "Greg Kroah-Hartman"
> CC: Chao Yu , Chao Yu
>
> Hi Gao,
>
> First bad commit (maybe != root cause):
>
> tree: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git
> staging-next
> head: 7bd50d785e30b290054b3a1b115393927da3
> commit: aca19723604c232ffc6376c1c6cd9c8a12158dcc [118/220] Revert "staging:
> erofs: disable compiling temporarile"
> :: branch date: 17 hours ago
> :: commit date: 25 hours ago
>
>>> drivers/staging/erofs/unzip_vle.c:1003:1-7: preceding lock on line 839
>
> #
> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/commit/?id=aca19723604c232ffc6376c1c6cd9c8a12158dcc
> git remote add staging
> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git
> git remote update staging
> git checkout aca19723604c232ffc6376c1c6cd9c8a12158dcc
> vim +1003 drivers/staging/erofs/unzip_vle.c
>
> 3883a79a Gao Xiang 2018-07-26 787
> 3883a79a Gao Xiang 2018-07-26 788 static int z_erofs_vle_unzip(struct
> super_block *sb,
> 3883a79a Gao Xiang 2018-07-26 789 struct z_erofs_vle_workgroup *grp,
> 3883a79a Gao Xiang 2018-07-26 790 struct list_head *page_pool)
> 3883a79a Gao Xiang 2018-07-26 791 {
> 3883a79a Gao Xiang 2018-07-26 792 struct erofs_sb_info *const sbi =
> EROFS_SB(sb);
> 105d4ad8 Gao Xiang 2018-07-26 793 #ifdef EROFS_FS_HAS_MANAGED_CACHE
> 105d4ad8 Gao Xiang 2018-07-26 794 struct address_space *const mngda =
> sbi->managed_cache->i_mapping;
> 105d4ad8 Gao Xiang 2018-07-26 795 #endif
> 3883a79a Gao Xiang 2018-07-26 796 const unsigned clusterpages =
> erofs_clusterpages(sbi);
> 3883a79a Gao Xiang 2018-07-26 797
> 3883a79a Gao Xiang 2018-07-26 798 struct z_erofs_pagevec_ctor ctor;
> 3883a79a Gao Xiang 2018-07-26 799 unsigned nr_pages;
> 3883a79a Gao Xiang 2018-07-26 800 #ifndef CONFIG_EROFS_FS_ZIP_MULTIREF
> 3883a79a Gao Xiang 2018-07-26 801 unsigned sparsemem_pages = 0;
> 3883a79a Gao Xiang 2018-07-26 802 #endif
> 3883a79a Gao Xiang 2018-07-26 803 struct page
> *pages_onstack[Z_EROFS_VLE_VMAP_ONSTACK_PAGES];
> 3883a79a Gao Xiang 2018-07-26 804 struct page **pages,
> **compressed_pages, *page;
> 3883a79a Gao Xiang 2018-07-26 805 unsigned i, llen;
> 3883a79a Gao Xiang 2018-07-26 806
> 3883a79a Gao Xiang 2018-07-26 807 enum z_erofs_page_type page_type;
> 3883a79a Gao Xiang 2018-07-26 808 bool overlapped;
> 3883a79a Gao Xiang 2018-07-26 809 struct z_erofs_vle_work *work;
> 3883a79a Gao Xiang 2018-07-26 810 void *vout;
> 3883a79a Gao Xiang 2018-07-26 811 int err;
> 3883a79a Gao Xiang 2018-07-26 812
> 3883a79a Gao Xiang 2018-07-26 813 might_sleep();
> 3883a79a Gao Xiang 2018-07-26 814 #ifndef CONFIG_EROFS_FS_ZIP_MULTIREF
> 3883a79a Gao Xiang 2018-07-26 815 work =
> z_erofs_vle_grab_primary_work(grp);
> 3883a79a Gao Xiang 2018-07-26 816 #else
> 3883a79a Gao Xiang 2018-07-26 817 BUG();
> 3883a79a Gao Xiang 2018-07-26 818 #endif
> 3883a79a Gao Xiang 2018-07-26 819 BUG_ON(!READ_ONCE(work->nr_pages));
> 3883a79a Gao Xiang 2018-07-26 820
> 3883a79a Gao Xiang 2018-07-26 821 mutex_lock(&work->lock);
> 3883a79a Gao Xiang 2018-07-26 822 nr_pages = work->nr_pages;
> 3883a79a Gao Xiang 2018-07-26 823
> 3883a79a Gao Xiang 2018-07-26 824 if (likely(nr_pages <=
> Z_EROFS_VLE_VMAP_ONSTACK_PAGES))
> 3883a79a Gao Xiang 2018-07-26 825 pages = pages_onstack;
> 3883a79a Gao Xiang 2018-07-26 826 else if (nr_pages <=
> Z_EROFS_VLE_VMAP_GLOBAL_PAGES &&
> 3883a79a Gao Xiang 2018-07-26 827
> mute
Re: [PATCH] erofs: option validation in remount
Hi Chengguang,
On 2018/9/11 23:37, cgxu519 wrote:
> On 09/11/2018 07:08 PM, Gao Xiang wrote:
>> Hi Chengguang,
>>
>> Thanks for your patch.
>>
>> The patch title should be "staging: erofs: " since erofs is still in staging.
> Hi Xiang,
>
> Thanks for your review. I'll add the tag from next version.
>
>>
>> The same as your previous patch "erofs: surround fault_injection ralted
>> option
>> parsing using CONFIG_EROFS_FAULT_INJECTION".
>>
>> On 2018/9/11 18:51, Chengguang Xu wrote:
>>> Add option validation in remount. After this patch, remount
>>> can change recognized options, and for unknown options remount
>>> will fail and report error.
>>>
>>> Signed-off-by: Chengguang Xu
>>> ---
>>> drivers/staging/erofs/super.c | 17 +
>>> 1 file changed, 17 insertions(+)
>>>
>>> diff --git a/drivers/staging/erofs/super.c b/drivers/staging/erofs/super.c
>>> index 1aec509c805f..8bab077381ad 100644
>>> --- a/drivers/staging/erofs/super.c
>>> +++ b/drivers/staging/erofs/super.c
>>> @@ -625,10 +625,27 @@ static int erofs_show_options(struct seq_file *seq,
>>> struct dentry *root)
>>> static int erofs_remount(struct super_block *sb, int *flags, char *data)
>>> {
>>> + struct erofs_sb_info *sbi = EROFS_SB(sb);
>>> + struct erofs_fault_info *ffi = &sbi->fault_info;
>>> + unsigned int orig_mount_opt = sbi->mount_opt;
>>> + unsigned int orig_inject_rate = ffi->inject_rate;
>>> + int err;
>>> +
>>> BUG_ON(!sb_rdonly(sb));
>>> + err = parse_options(sb, data);
>>> + if (err)
>>> + goto out;
>>> +
>>> *flags |= MS_RDONLY;
>>> return 0;
>>> +
>>> +out:
>>> + if (ffi->inject_rate != orig_inject_rate)
>>> + erofs_build_fault_attr(sbi, orig_inject_rate);
>> Currently should be with "#ifdef CONFIG_EROFS_FAULT_INJECTION"?
>> and have you tried to compile without EROFS_FAULT_INJECTION?
> Ah, It must be enabled in my environment.
> However, adding the macro in every calling place seems not convenient,
> I'll try to do some code cleanups, so please hold on this and previous patch
> for
> a moment, I'll resend those in a patch set.
>
OK, that is fine. look forword to your next patch. ;)
Thanks,
Gao Xiang
> Thanks,
> Chengguang
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4] staging: erofs: use explicit unsigned int type
Hi Thomas, On 2018/9/11 3:41, Thomas Weißschuh wrote: > Hi Chao, > > On Mon, 2018-09-10T23:59+0800, Chao Yu wrote: >> [...] >>>> I was not aware of this tree and worked off of staging / next. >>>> A patch is attached to this message that adds the tree to the MAINTAINERS >>>> file. >>> Hi Chao, >>> >>> I think this tree has some PREVIEW patches which preview in linux-erofs >>> mailing list only and >>> doesn't send to staging mailing list and LKML, >>> >>> https://git.kernel.org/pub/scm/linux/kernel/git/chao/linux.git/log/?h=erofs >>> >>> so erofs tree is actually Greg's staging tree. >> Thomas, >> >> I confirmed that erofs git repository for linux upstream is Greg's staging >> tree. >> >> Let me explain, in order to avoid sending buggy or preview patch, Xiang and >> me >> plan to review patches in erofs mailing list first, and then cache reviewed >> patches in my git tree before sending them to Greg and staging mailing list. >> >> Based on that, I'm trying to serialize all erofs patches, expecting that can >> help those patches sent to staging mailing list can be merged by Greg with >> lesser conflict. But I made a mistake that my erofs branch has merged some >> pending patches, result in failing to merge yours, that mislead me to ask >> you to >> rebase the code, sorry about that. > Thank you for clearing this up! And I am sorry for causing you all this work > for what is essentially a very small style cleanup. > >> Now I can confirm that your v2 patch can apply on Greg's staging-next, so >> fixing >> warning reported by checkpatch.pl on your v2 patch is enough. :) > The patch follows. > > Thomas Could you please resend your patch seperately? Because it will be easier for Greg to merge. > > > Changes since v1: > > * Removed changes that conflicted with > [PATCH 1/6] staging: erofs: formatting fix in unzip_vle_lz4.c > * Added patch description > > Changes since v2: > > * Fixed conflicts with other patchsets > * Don't introduce new style issues > > Changes since v3: > > * Fixed conflicts with other patchsets > > Note: This patchset should be applied with the "git am --scissors", to > remove the historic information and this note. > > -- >8 -- I personally think that is not the correct kernel patch style. Just as Greg's said, > These changes belong below the --- line, not above it. LINK: https://lists.ozlabs.org/pipermail/linux-erofs/2018-August/000367.html For reference, it will help the patch quickly get merged. ;) and you could add, Reviewed-by: Gao Xiang Thanks, Gao Xiang > > Fix coding style issue "Prefer 'unsigned int' to bare use of 'unsigned'" > detected by checkpatch.pl. > > Signed-off-by: Thomas Weißschuh ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/7] staging: erofs: introduce a new helper __erofs_build_fault_attr()
Hi Chengguang,
On 2018/9/12 13:10, Chengguang Xu wrote:
> Introduce a new helper __erofs_build_fault_attr() to handle set/clear
> erofs_fault_info, we need this funciton for internal use case.
> for example, reset fault_injection option in remount.
>
> Signed-off-by: Chengguang Xu
> ---
> drivers/staging/erofs/super.c | 26 ++
> 1 file changed, 18 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/staging/erofs/super.c b/drivers/staging/erofs/super.c
> index 14dbb6517b8d..d2dbc1fd3abb 100644
> --- a/drivers/staging/erofs/super.c
> +++ b/drivers/staging/erofs/super.c
> @@ -144,15 +144,9 @@ char *erofs_fault_name[FAULT_MAX] = {
> [FAULT_KMALLOC] = "kmalloc",
> };
>
> -static int erofs_build_fault_attr(struct erofs_sb_info *sbi,
> - substring_t *args)
> +static void __erofs_build_fault_attr(struct erofs_sb_info *sbi,
> + unsigned int rate)
> {
> - struct erofs_fault_info *ffi = &sbi->fault_info;
> - int rate = 0;
> -
> - if (args->from && match_int(args, &rate))
> - return -EINVAL;
> -
> if (rate) {
I get some compile error of this patch...
drivers/staging/erofs/super.c: In function ‘__erofs_build_fault_attr’:
drivers/staging/erofs/super.c:156:15: error: ‘ffi’ undeclared (first use in
this function)
atomic_set(&ffi->inject_ops, 0);
^
drivers/staging/erofs/super.c:156:15: note: each undeclared identifier is
reported only once for each function it appears in
drivers/staging/erofs/super.c: In function ‘erofs_build_fault_attr’:
drivers/staging/erofs/super.c:169:27: warning: unused variable ‘ffi’
[-Wunused-variable]
struct erofs_fault_info *ffi = &sbi->fault_info;
p.s. could you please rebase your patch on Thomas's [PATCH v4] staging: erofs:
use explicit unsigned int type ?
since I'm rebasing the rest PREVIEW patches on this commit now.
p.p.s. I'd like to get Chao's idea of this fault injection patchset first :)
Thanks,
Gao Xiang
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/7] staging: erofs: introduce a new helper __erofs_build_fault_attr()
On 2018/9/12 22:23, cgxu519 wrote:
> On 09/12/2018 07:22 PM, Gao Xiang wrote:
>> Hi Chengguang,
>>
>> On 2018/9/12 13:10, Chengguang Xu wrote:
>>> Introduce a new helper __erofs_build_fault_attr() to handle set/clear
>>> erofs_fault_info, we need this funciton for internal use case.
>>> for example, reset fault_injection option in remount.
>>>
>>> Signed-off-by: Chengguang Xu
>>> ---
>>> drivers/staging/erofs/super.c | 26 ++
>>> 1 file changed, 18 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/staging/erofs/super.c b/drivers/staging/erofs/super.c
>>> index 14dbb6517b8d..d2dbc1fd3abb 100644
>>> --- a/drivers/staging/erofs/super.c
>>> +++ b/drivers/staging/erofs/super.c
>>> @@ -144,15 +144,9 @@ char *erofs_fault_name[FAULT_MAX] = {
>>> [FAULT_KMALLOC] = "kmalloc",
>>> };
>>> -static int erofs_build_fault_attr(struct erofs_sb_info *sbi,
>>> - substring_t *args)
>>> +static void __erofs_build_fault_attr(struct erofs_sb_info *sbi,
>>> + unsigned int rate)
>>> {
>>> - struct erofs_fault_info *ffi = &sbi->fault_info;
>>> - int rate = 0;
>>> -
>>> - if (args->from && match_int(args, &rate))
>>> - return -EINVAL;
>>> -
>>> if (rate) {
>> I get some compile error of this patch...
>> drivers/staging/erofs/super.c: In function ‘__erofs_build_fault_attr’:
>> drivers/staging/erofs/super.c:156:15: error: ‘ffi’ undeclared (first use in
>> this function)
>> atomic_set(&ffi->inject_ops, 0);
>> ^
>> drivers/staging/erofs/super.c:156:15: note: each undeclared identifier is
>> reported only once for each function it appears in
>> drivers/staging/erofs/super.c: In function ‘erofs_build_fault_attr’:
>> drivers/staging/erofs/super.c:169:27: warning: unused variable ‘ffi’
>> [-Wunused-variable]
>> struct erofs_fault_info *ffi = &sbi->fault_info;
> Sorry for that, I'll fix it in rebased version.
>
>>
>> p.s. could you please rebase your patch on Thomas's [PATCH v4] staging:
>> erofs: use explicit unsigned int type ?
>> since I'm rebasing the rest PREVIEW patches on this commit now.
>
> I noticed Thomas' patch had already committed to erofs-master branch in
> Chao's linux git repo, also my
> previous patch but not in erofs-dev branch. So should I rebase on
> erofs-master?
> Could you give me a little more guide for it?
Hi Chengguang,
Recently many cleanup patches submitted to Greg's staging tree and I need to
rebase the rest erofs preview patches for linux-4.20 on these accepted
cleanup patches.
I think you could make your patch based on Thomas's patch (erofs-master
branch in Chao's linux git repo), you could also tell Chao drop your
previous patch.
Thanks,
Gao Xiang
>>
>> p.p.s. I'd like to get Chao's idea of this fault injection patchset first :)
> No problem, let's wait for a while, then I'll rebase the code according to
> the comments.
>
>
> Thanks,
> Chengguang
>
> ___
> devel mailing list
> de...@linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4] staging: erofs: use explicit unsigned int type
Hi Thomas, ping... On 2018/9/12 14:21, Gao Xiang wrote: > Hi Thomas, > > On 2018/9/11 3:41, Thomas Weißschuh wrote: >> Hi Chao, >> >> On Mon, 2018-09-10T23:59+0800, Chao Yu wrote: >>> [...] >>>>> I was not aware of this tree and worked off of staging / next. >>>>> A patch is attached to this message that adds the tree to the MAINTAINERS >>>>> file. >>>> Hi Chao, >>>> >>>> I think this tree has some PREVIEW patches which preview in linux-erofs >>>> mailing list only and >>>> doesn't send to staging mailing list and LKML, >>>> >>>> https://git.kernel.org/pub/scm/linux/kernel/git/chao/linux.git/log/?h=erofs >>>> >>>> so erofs tree is actually Greg's staging tree. >>> Thomas, >>> >>> I confirmed that erofs git repository for linux upstream is Greg's staging >>> tree. >>> >>> Let me explain, in order to avoid sending buggy or preview patch, Xiang and >>> me >>> plan to review patches in erofs mailing list first, and then cache reviewed >>> patches in my git tree before sending them to Greg and staging mailing list. >>> >>> Based on that, I'm trying to serialize all erofs patches, expecting that can >>> help those patches sent to staging mailing list can be merged by Greg with >>> lesser conflict. But I made a mistake that my erofs branch has merged some >>> pending patches, result in failing to merge yours, that mislead me to ask >>> you to >>> rebase the code, sorry about that. >> Thank you for clearing this up! And I am sorry for causing you all this work >> for what is essentially a very small style cleanup. >> >>> Now I can confirm that your v2 patch can apply on Greg's staging-next, so >>> fixing >>> warning reported by checkpatch.pl on your v2 patch is enough. :) >> The patch follows. >> >> Thomas > > Could you please resend your patch seperately? Because it will be easier for > Greg to merge. > >> >> >> Changes since v1: >> >> * Removed changes that conflicted with >> [PATCH 1/6] staging: erofs: formatting fix in unzip_vle_lz4.c >> * Added patch description >> >> Changes since v2: >> >> * Fixed conflicts with other patchsets >> * Don't introduce new style issues >> >> Changes since v3: >> >> * Fixed conflicts with other patchsets >> >> Note: This patchset should be applied with the "git am --scissors", to >> remove the historic information and this note. >> >> -- >8 -- > > I personally think that is not the correct kernel patch style. > > Just as Greg's said, >> These changes belong below the --- line, not above it. > > LINK: https://lists.ozlabs.org/pipermail/linux-erofs/2018-August/000367.html > > For reference, it will help the patch quickly get merged. ;) > > and you could add, > Reviewed-by: Gao Xiang > > Thanks, > Gao Xiang > and HUAWEI email server cannot send my all emails to your mailbox successfully. Since you change a lot of files,I need to rebase my rest preview patches one by one right now. Could you please resend a acceptable independent final patch for Greg? :) Thanks, Gao Xiang >> >> Fix coding style issue "Prefer 'unsigned int' to bare use of 'unsigned'" >> detected by checkpatch.pl. >> >> Signed-off-by: Thomas Weißschuh > ___ > devel mailing list > de...@linuxdriverproject.org > http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 0/8] staging: erofs: error handing and more tracepoints
In order to avoid conflicts with cleanup patches, Chao and I think it is better to send reviewed preview patches in the erofs mailing list to the community in time. So here is reviewed & tested patches right now, which clean up and enhance the error handing and add some tracepoints for decompression. Note that in this patchset, bare use of 'unsigned' and NULL comparison are also fixed compared with the preview patches according to the previous discussion in the staging mailing list. Thanks, Gao Xiang Chen Gong (2): staging: erofs: add trace points for reading zipped data staging: erofs: replace BUG_ON with DBG_BUGON in data.c Gao Xiang (6): staging: erofs: fix a missing endian conversion staging: erofs: clean up z_erofs_map_blocks_iter staging: erofs: complete error handing of z_erofs_map_blocks_iter staging: erofs: fix a bug when appling cache strategy staging: erofs: complete error handing of z_erofs_do_read_page staging: erofs: avoid magic constants when initializing clusterbits drivers/staging/erofs/data.c | 31 ++-- drivers/staging/erofs/include/trace/events/erofs.h | 20 ++- drivers/staging/erofs/super.c | 5 +- drivers/staging/erofs/unzip_vle.c | 195 + 4 files changed, 162 insertions(+), 89 deletions(-) -- 1.9.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 1/8] staging: erofs: fix a missing endian conversion
This patch fixes a missing endian conversion in
vle_get_logical_extent_head.
Reviewed-by: Chao Yu
Signed-off-by: Gao Xiang
---
drivers/staging/erofs/unzip_vle.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/erofs/unzip_vle.c
b/drivers/staging/erofs/unzip_vle.c
index 21874b7..d16e3dc 100644
--- a/drivers/staging/erofs/unzip_vle.c
+++ b/drivers/staging/erofs/unzip_vle.c
@@ -1488,6 +1488,7 @@ static erofs_off_t vle_get_logical_extent_head(
struct super_block *const sb = inode->i_sb;
const unsigned int clusterbits = EROFS_SB(sb)->clusterbits;
const unsigned int clustersize = 1 << clusterbits;
+ unsigned int delta0;
if (page->index != blkaddr) {
kunmap_atomic(*kaddr_iter);
@@ -1502,12 +1503,13 @@ static erofs_off_t vle_get_logical_extent_head(
di = *kaddr_iter + vle_extent_blkoff(inode, lcn);
switch (vle_cluster_type(di)) {
case Z_EROFS_VLE_CLUSTER_TYPE_NONHEAD:
- BUG_ON(!di->di_u.delta[0]);
- BUG_ON(lcn < di->di_u.delta[0]);
+ delta0 = le16_to_cpu(di->di_u.delta[0]);
+ DBG_BUGON(!delta0);
+ DBG_BUGON(lcn < delta0);
ofs = vle_get_logical_extent_head(inode,
page_iter, kaddr_iter,
- lcn - di->di_u.delta[0], pcn, flags);
+ lcn - delta0, pcn, flags);
break;
case Z_EROFS_VLE_CLUSTER_TYPE_PLAIN:
*flags ^= EROFS_MAP_ZIPPED;
--
1.9.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 2/8] staging: erofs: clean up z_erofs_map_blocks_iter
This patch mainly introduces `vle_map_blocks_iter_ctx' to clean up
z_erofs_map_blocks_iter and vle_get_logical_extent_head.
It changes the return value of `vle_get_logical_extent_head' to int
for the later error handing. In addition, it also renames `pcn' to
`pblk' since only `pblk' exists in erofs compression ondisk format.
Reviewed-by: Chao Yu
Signed-off-by: Gao Xiang
---
drivers/staging/erofs/unzip_vle.c | 139 +-
1 file changed, 77 insertions(+), 62 deletions(-)
diff --git a/drivers/staging/erofs/unzip_vle.c
b/drivers/staging/erofs/unzip_vle.c
index d16e3dc..d6e5956 100644
--- a/drivers/staging/erofs/unzip_vle.c
+++ b/drivers/staging/erofs/unzip_vle.c
@@ -1410,6 +1410,13 @@ static int z_erofs_vle_normalaccess_readpages(
.readpages = z_erofs_vle_normalaccess_readpages,
};
+/*
+ * Variable-sized Logical Extent (Fixed Physical Cluster) Compression Mode
+ * ---
+ * VLE compression mode attempts to compress a number of logical data into
+ * a physical cluster with a fixed size.
+ * VLE compression mode uses "struct z_erofs_vle_decompressed_index".
+ */
#define __vle_cluster_advise(x, bit, bits) \
((le16_to_cpu(x) >> (bit)) & ((1 << (bits)) - 1))
@@ -1465,90 +1472,95 @@ static int z_erofs_vle_normalaccess_readpages(
return erofs_blkoff(iloc(sbi, vi->nid) + ofs);
}
-/*
- * Variable-sized Logical Extent (Fixed Physical Cluster) Compression Mode
- * ---
- * VLE compression mode attempts to compress a number of logical data into
- * a physical cluster with a fixed size.
- * VLE compression mode uses "struct z_erofs_vle_decompressed_index".
- */
-static erofs_off_t vle_get_logical_extent_head(
- struct inode *inode,
- struct page **page_iter,
- void **kaddr_iter,
- unsigned int lcn, /* logical cluster number */
- erofs_blk_t *pcn,
- unsigned int *flags)
+struct vle_map_blocks_iter_ctx {
+ struct inode *inode;
+ struct super_block *sb;
+ unsigned int clusterbits;
+
+ struct page **mpage_ret;
+ void **kaddr_ret;
+};
+
+static int
+vle_get_logical_extent_head(const struct vle_map_blocks_iter_ctx *ctx,
+ unsigned int lcn, /* logical cluster number */
+ unsigned long long *ofs,
+ erofs_blk_t *pblk,
+ unsigned int *flags)
{
- /* for extent meta */
- struct page *page = *page_iter;
- erofs_blk_t blkaddr = vle_extent_blkaddr(inode, lcn);
+ const unsigned int clustersize = 1 << ctx->clusterbits;
+ const erofs_blk_t mblk = vle_extent_blkaddr(ctx->inode, lcn);
+ struct page *mpage = *ctx->mpage_ret; /* extent metapage */
+
struct z_erofs_vle_decompressed_index *di;
- unsigned long long ofs;
- struct super_block *const sb = inode->i_sb;
- const unsigned int clusterbits = EROFS_SB(sb)->clusterbits;
- const unsigned int clustersize = 1 << clusterbits;
- unsigned int delta0;
-
- if (page->index != blkaddr) {
- kunmap_atomic(*kaddr_iter);
- unlock_page(page);
- put_page(page);
+ unsigned int cluster_type, delta0;
- page = erofs_get_meta_page_nofail(sb, blkaddr, false);
- *page_iter = page;
- *kaddr_iter = kmap_atomic(page);
+ if (mpage->index != mblk) {
+ kunmap_atomic(*ctx->kaddr_ret);
+ unlock_page(mpage);
+ put_page(mpage);
+
+ mpage = erofs_get_meta_page_nofail(ctx->sb, mblk, false);
+ *ctx->mpage_ret = mpage;
+ *ctx->kaddr_ret = kmap_atomic(mpage);
}
- di = *kaddr_iter + vle_extent_blkoff(inode, lcn);
- switch (vle_cluster_type(di)) {
+ di = *ctx->kaddr_ret + vle_extent_blkoff(ctx->inode, lcn);
+
+ cluster_type = vle_cluster_type(di);
+ switch (cluster_type) {
case Z_EROFS_VLE_CLUSTER_TYPE_NONHEAD:
delta0 = le16_to_cpu(di->di_u.delta[0]);
DBG_BUGON(!delta0);
DBG_BUGON(lcn < delta0);
- ofs = vle_get_logical_extent_head(inode,
- page_iter, kaddr_iter,
- lcn - delta0, pcn, flags);
- break;
+ return vle_get_logical_extent_head(ctx,
+ lcn - delta0, ofs, pblk, flags);
case Z_EROFS_VLE_CLUSTER_TYPE_PLAIN:
*flags ^= EROFS_MAP_ZIPPED;
+ /* fallthrough */
case Z_EROFS_VLE_CLUSTER_TYPE_HEAD:
/* clustersize should be a power of two */
- ofs = ((u64)lcn << clusterbits) +
+ *ofs = ((u64)lcn << ctx->clusterbits) +
(le16_to_cpu(di->di_clusterofs) & (clustersize - 1));
-
[PATCH 3/8] staging: erofs: complete error handing of z_erofs_map_blocks_iter
This patch completes error handing of z_erofs_map_blocks_iter
and vle_get_logical_extent_head, including no memory and
io error cases.
Reviewed-by: Chao Yu
Signed-off-by: Gao Xiang
---
drivers/staging/erofs/unzip_vle.c | 35 ++-
1 file changed, 26 insertions(+), 9 deletions(-)
diff --git a/drivers/staging/erofs/unzip_vle.c
b/drivers/staging/erofs/unzip_vle.c
index d6e5956..d4998fe 100644
--- a/drivers/staging/erofs/unzip_vle.c
+++ b/drivers/staging/erofs/unzip_vle.c
@@ -1500,7 +1500,11 @@ struct vle_map_blocks_iter_ctx {
unlock_page(mpage);
put_page(mpage);
- mpage = erofs_get_meta_page_nofail(ctx->sb, mblk, false);
+ mpage = erofs_get_meta_page(ctx->sb, mblk, false);
+ if (IS_ERR(mpage)) {
+ *ctx->mpage_ret = NULL;
+ return PTR_ERR(mpage);
+ }
*ctx->mpage_ret = mpage;
*ctx->kaddr_ret = kmap_atomic(mpage);
}
@@ -1511,9 +1515,12 @@ struct vle_map_blocks_iter_ctx {
switch (cluster_type) {
case Z_EROFS_VLE_CLUSTER_TYPE_NONHEAD:
delta0 = le16_to_cpu(di->di_u.delta[0]);
- DBG_BUGON(!delta0);
- DBG_BUGON(lcn < delta0);
-
+ if (unlikely(!delta0 || delta0 > lcn)) {
+ errln("invalid NONHEAD dl0 %u at lcn %u of nid %llu",
+ delta0, lcn, EROFS_V(ctx->inode)->nid);
+ DBG_BUGON(1);
+ return -EIO;
+ }
return vle_get_logical_extent_head(ctx,
lcn - delta0, ofs, pblk, flags);
case Z_EROFS_VLE_CLUSTER_TYPE_PLAIN:
@@ -1526,7 +1533,10 @@ struct vle_map_blocks_iter_ctx {
*pblk = le32_to_cpu(di->di_u.blkaddr);
break;
default:
- BUG_ON(1);
+ errln("unknown cluster type %u at lcn %u of nid %llu",
+ cluster_type, lcn, EROFS_V(ctx->inode)->nid);
+ DBG_BUGON(1);
+ return -EIO;
}
return 0;
}
@@ -1582,7 +1592,11 @@ int z_erofs_map_blocks_iter(struct inode *inode,
if (mpage != NULL)
put_page(mpage);
- mpage = erofs_get_meta_page_nofail(ctx.sb, mblk, false);
+ mpage = erofs_get_meta_page(ctx.sb, mblk, false);
+ if (IS_ERR(mpage)) {
+ err = PTR_ERR(mpage);
+ goto out;
+ }
*mpage_ret = mpage;
} else {
lock_page(mpage);
@@ -1645,8 +1659,11 @@ int z_erofs_map_blocks_iter(struct inode *inode,
&pblk, &map->m_flags);
mpage = *mpage_ret;
- if (unlikely(err))
- goto unmap_out;
+ if (unlikely(err)) {
+ if (mpage)
+ goto unmap_out;
+ goto out;
+ }
break;
default:
errln("unknown cluster type %u at offset %llu of nid %llu",
@@ -1670,7 +1687,7 @@ int z_erofs_map_blocks_iter(struct inode *inode,
map->m_llen, map->m_plen, map->m_flags);
/* aggressively BUG_ON iff CONFIG_EROFS_FS_DEBUG is on */
- DBG_BUGON(err < 0);
+ DBG_BUGON(err < 0 && err != -ENOMEM);
return err;
}
--
1.9.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 4/8] staging: erofs: fix a bug when appling cache strategy
As described in Kconfig, the last compressed pack should be cached
for further reading for either `EROFS_FS_ZIP_CACHE_UNIPOLAR' or
`EROFS_FS_ZIP_CACHE_BIPOLAR' by design.
However, there is a bug in z_erofs_do_read_page, it will
switch `initial' to `false' at the very beginning before it decides
to cache the last compressed pack.
caching strategy should work properly after appling this patch.
Reviewed-by: Chao Yu
Signed-off-by: Gao Xiang
---
drivers/staging/erofs/unzip_vle.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/erofs/unzip_vle.c
b/drivers/staging/erofs/unzip_vle.c
index d4998fe..0f2e180 100644
--- a/drivers/staging/erofs/unzip_vle.c
+++ b/drivers/staging/erofs/unzip_vle.c
@@ -629,7 +629,7 @@ static int z_erofs_do_read_page(struct z_erofs_vle_frontend
*fe,
/* go ahead the next map_blocks */
debugln("%s: [out-of-range] pos %llu", __func__, offset + cur);
- if (!z_erofs_vle_work_iter_end(builder))
+ if (z_erofs_vle_work_iter_end(builder))
fe->initial = false;
map->m_la = offset + cur;
--
1.9.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 5/8] staging: erofs: complete error handing of z_erofs_do_read_page
This patch completes error handing code of z_erofs_do_read_page.
PG_error will be set when some read error happens, therefore
z_erofs_onlinepage_endio will unlock this page without setting
PG_uptodate.
Reviewed-by: Chao Yu
Signed-off-by: Gao Xiang
---
drivers/staging/erofs/unzip_vle.c | 20 +++-
1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/drivers/staging/erofs/unzip_vle.c
b/drivers/staging/erofs/unzip_vle.c
index 0f2e180..15c2015 100644
--- a/drivers/staging/erofs/unzip_vle.c
+++ b/drivers/staging/erofs/unzip_vle.c
@@ -611,7 +611,7 @@ static int z_erofs_do_read_page(struct z_erofs_vle_frontend
*fe,
enum z_erofs_page_type page_type;
unsigned int cur, end, spiltted, index;
- int err;
+ int err = 0;
/* register locked file pages as online pages in pack */
z_erofs_onlinepage_init(page);
@@ -638,12 +638,11 @@ static int z_erofs_do_read_page(struct
z_erofs_vle_frontend *fe,
if (unlikely(err))
goto err_out;
- /* deal with hole (FIXME! broken now) */
if (unlikely(!(map->m_flags & EROFS_MAP_MAPPED)))
goto hitted;
DBG_BUGON(map->m_plen != 1 << sbi->clusterbits);
- BUG_ON(erofs_blkoff(map->m_pa));
+ DBG_BUGON(erofs_blkoff(map->m_pa));
err = z_erofs_vle_work_iter_begin(builder, sb, map, &fe->owned_head);
if (unlikely(err))
@@ -688,7 +687,7 @@ static int z_erofs_do_read_page(struct z_erofs_vle_frontend
*fe,
err = z_erofs_vle_work_add_page(builder,
newpage, Z_EROFS_PAGE_TYPE_EXCLUSIVE);
- if (!err)
+ if (likely(!err))
goto retry;
}
@@ -699,9 +698,10 @@ static int z_erofs_do_read_page(struct
z_erofs_vle_frontend *fe,
/* FIXME! avoid the last relundant fixup & endio */
z_erofs_onlinepage_fixup(page, index, true);
- ++spiltted;
- /* also update nr_pages and increase queued_pages */
+ /* bump up the number of spiltted parts of a page */
+ ++spiltted;
+ /* also update nr_pages */
work->nr_pages = max_t(pgoff_t, work->nr_pages, index + 1);
next_part:
/* can be used for verification */
@@ -711,16 +711,18 @@ static int z_erofs_do_read_page(struct
z_erofs_vle_frontend *fe,
if (end > 0)
goto repeat;
+out:
/* FIXME! avoid the last relundant fixup & endio */
z_erofs_onlinepage_endio(page);
debugln("%s, finish page: %pK spiltted: %u map->m_llen %llu",
__func__, page, spiltted, map->m_llen);
- return 0;
+ return err;
+ /* if some error occurred while processing this page */
err_out:
- /* TODO: the missing error handing cases */
- return err;
+ SetPageError(page);
+ goto out;
}
static void z_erofs_vle_unzip_kickoff(void *ptr, int bios)
--
1.9.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 6/8] staging: erofs: avoid magic constants when initializing clusterbits
Currently erofs only supports clustersize == blocksize.
and clustersize == 2^n * blocksize will be supported in the future.
Reviewed-by: Chao Yu
Signed-off-by: Gao Xiang
---
drivers/staging/erofs/super.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/erofs/super.c b/drivers/staging/erofs/super.c
index 2109b03..9e42153 100644
--- a/drivers/staging/erofs/super.c
+++ b/drivers/staging/erofs/super.c
@@ -116,9 +116,10 @@ static int superblock_read(struct super_block *sb)
#endif
sbi->islotbits = ffs(sizeof(struct erofs_inode_v1)) - 1;
#ifdef CONFIG_EROFS_FS_ZIP
- sbi->clusterbits = 12;
+ /* TODO: clusterbits should be related to inode */
+ sbi->clusterbits = blkszbits;
- if (1 << (sbi->clusterbits - 12) > Z_EROFS_CLUSTER_MAX_PAGES)
+ if (1 << (sbi->clusterbits - PAGE_SHIFT) > Z_EROFS_CLUSTER_MAX_PAGES)
errln("clusterbits %u is not supported on this kernel",
sbi->clusterbits);
#endif
--
1.9.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 7/8] staging: erofs: add trace points for reading zipped data
From: Chen Gong
This patch adds trace points for reading zipped data.
Signed-off-by: Chen Gong
Reviewed-by: Chao Yu
Reviewed-by: Gao Xiang
Signed-off-by: Gao Xiang
---
drivers/staging/erofs/include/trace/events/erofs.h | 20 ++--
drivers/staging/erofs/unzip_vle.c | 11 +++
2 files changed, 29 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/erofs/include/trace/events/erofs.h
b/drivers/staging/erofs/include/trace/events/erofs.h
index 5aead93..660c92f 100644
--- a/drivers/staging/erofs/include/trace/events/erofs.h
+++ b/drivers/staging/erofs/include/trace/events/erofs.h
@@ -162,7 +162,8 @@
TP_printk("dev = (%d,%d), nid = %llu, la %llu llen %llu flags %s",
show_dev_nid(__entry),
- __entry->la, __entry->llen, show_map_flags(__entry->flags))
+ __entry->la, __entry->llen,
+ __entry->flags ? show_map_flags(__entry->flags) : "NULL")
);
DEFINE_EVENT(erofs__map_blocks_enter, erofs_map_blocks_flatmode_enter,
@@ -172,6 +173,13 @@
TP_ARGS(inode, map, flags)
);
+DEFINE_EVENT(erofs__map_blocks_enter, z_erofs_map_blocks_iter_enter,
+ TP_PROTO(struct inode *inode, struct erofs_map_blocks *map,
+unsigned int flags),
+
+ TP_ARGS(inode, map, flags)
+);
+
DECLARE_EVENT_CLASS(erofs__map_blocks_exit,
TP_PROTO(struct inode *inode, struct erofs_map_blocks *map,
unsigned int flags, int ret),
@@ -204,7 +212,8 @@
TP_printk("dev = (%d,%d), nid = %llu, flags %s "
"la %llu pa %llu llen %llu plen %llu mflags %s ret %d",
- show_dev_nid(__entry), show_map_flags(__entry->flags),
+ show_dev_nid(__entry),
+ __entry->flags ? show_map_flags(__entry->flags) : "NULL",
__entry->la, __entry->pa, __entry->llen, __entry->plen,
show_mflags(__entry->mflags), __entry->ret)
);
@@ -216,6 +225,13 @@
TP_ARGS(inode, map, flags, ret)
);
+DEFINE_EVENT(erofs__map_blocks_exit, z_erofs_map_blocks_iter_exit,
+ TP_PROTO(struct inode *inode, struct erofs_map_blocks *map,
+unsigned int flags, int ret),
+
+ TP_ARGS(inode, map, flags, ret)
+);
+
TRACE_EVENT(erofs_destroy_inode,
TP_PROTO(struct inode *inode),
diff --git a/drivers/staging/erofs/unzip_vle.c
b/drivers/staging/erofs/unzip_vle.c
index 15c2015..ad3b7bb 100644
--- a/drivers/staging/erofs/unzip_vle.c
+++ b/drivers/staging/erofs/unzip_vle.c
@@ -13,6 +13,8 @@
#include "unzip_vle.h"
#include
+#include
+
static struct workqueue_struct *z_erofs_workqueue __read_mostly;
static struct kmem_cache *z_erofs_workgroup_cachep __read_mostly;
@@ -613,6 +615,8 @@ static int z_erofs_do_read_page(struct z_erofs_vle_frontend
*fe,
unsigned int cur, end, spiltted, index;
int err = 0;
+ trace_erofs_readpage(page, false);
+
/* register locked file pages as online pages in pack */
z_erofs_onlinepage_init(page);
@@ -1348,6 +1352,9 @@ static inline int __z_erofs_vle_normalaccess_readpages(
struct page *head = NULL;
LIST_HEAD(pagepool);
+ trace_erofs_readpages(mapping->host, lru_to_page(pages),
+ nr_pages, false);
+
#if (EROFS_FS_ZIP_CACHE_LVL >= 2)
f.cachedzone_la = (erofs_off_t)lru_to_page(pages)->index << PAGE_SHIFT;
#endif
@@ -1570,6 +1577,8 @@ int z_erofs_map_blocks_iter(struct inode *inode,
unsigned int cluster_type, logical_cluster_ofs;
int err = 0;
+ trace_z_erofs_map_blocks_iter_enter(inode, map, flags);
+
/* when trying to read beyond EOF, leave it unmapped */
if (unlikely(map->m_la >= inode->i_size)) {
DBG_BUGON(!initial);
@@ -1688,6 +1697,8 @@ int z_erofs_map_blocks_iter(struct inode *inode,
__func__, map->m_la, map->m_pa,
map->m_llen, map->m_plen, map->m_flags);
+ trace_z_erofs_map_blocks_iter_exit(inode, map, flags, err);
+
/* aggressively BUG_ON iff CONFIG_EROFS_FS_DEBUG is on */
DBG_BUGON(err < 0 && err != -ENOMEM);
return err;
--
1.9.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 8/8] staging: erofs: replace BUG_ON with DBG_BUGON in data.c
From: Chen Gong
This patch replace BUG_ON with DBG_BUGON in data.c, and add necessary
error handler.
Signed-off-by: Chen Gong
Reviewed-by: Gao Xiang
Reviewed-by: Chao Yu
Signed-off-by: Gao Xiang
---
drivers/staging/erofs/data.c | 31 ---
1 file changed, 20 insertions(+), 11 deletions(-)
diff --git a/drivers/staging/erofs/data.c b/drivers/staging/erofs/data.c
index e191610..6384f73 100644
--- a/drivers/staging/erofs/data.c
+++ b/drivers/staging/erofs/data.c
@@ -25,7 +25,7 @@ static inline void read_endio(struct bio *bio)
struct page *page = bvec->bv_page;
/* page is already locked */
- BUG_ON(PageUptodate(page));
+ DBG_BUGON(PageUptodate(page));
if (unlikely(err))
SetPageError(page);
@@ -110,12 +110,12 @@ static int erofs_map_blocks_flatmode(struct inode *inode,
struct erofs_map_blocks *map,
int flags)
{
+ int err = 0;
erofs_blk_t nblocks, lastblk;
u64 offset = map->m_la;
struct erofs_vnode *vi = EROFS_V(inode);
trace_erofs_map_blocks_flatmode_enter(inode, map, flags);
- BUG_ON(is_inode_layout_compression(inode));
nblocks = DIV_ROUND_UP(inode->i_size, PAGE_SIZE);
lastblk = nblocks - is_inode_layout_inline(inode);
@@ -142,18 +142,27 @@ static int erofs_map_blocks_flatmode(struct inode *inode,
map->m_plen = inode->i_size - offset;
/* inline data should locate in one meta block */
- BUG_ON(erofs_blkoff(map->m_pa) + map->m_plen > PAGE_SIZE);
+ if (erofs_blkoff(map->m_pa) + map->m_plen > PAGE_SIZE) {
+ DBG_BUGON(1);
+ err = -EIO;
+ goto err_out;
+ }
+
map->m_flags |= EROFS_MAP_META;
} else {
errln("internal error @ nid: %llu (size %llu), m_la 0x%llx",
vi->nid, inode->i_size, map->m_la);
- BUG();
+ DBG_BUGON(1);
+ err = -EIO;
+ goto err_out;
}
out:
map->m_llen = map->m_plen;
+
+err_out:
trace_erofs_map_blocks_flatmode_exit(inode, map, flags, 0);
- return 0;
+ return err;
}
#ifdef CONFIG_EROFS_FS_ZIP
@@ -209,7 +218,7 @@ static inline struct bio *erofs_read_raw_page(
erofs_off_t current_block = (erofs_off_t)page->index;
int err;
- BUG_ON(!nblocks);
+ DBG_BUGON(!nblocks);
if (PageUptodate(page)) {
err = 0;
@@ -252,7 +261,7 @@ static inline struct bio *erofs_read_raw_page(
}
/* for RAW access mode, m_plen must be equal to m_llen */
- BUG_ON(map.m_plen != map.m_llen);
+ DBG_BUGON(map.m_plen != map.m_llen);
blknr = erofs_blknr(map.m_pa);
blkoff = erofs_blkoff(map.m_pa);
@@ -262,7 +271,7 @@ static inline struct bio *erofs_read_raw_page(
void *vsrc, *vto;
struct page *ipage;
- BUG_ON(map.m_plen > PAGE_SIZE);
+ DBG_BUGON(map.m_plen > PAGE_SIZE);
ipage = erofs_get_meta_page(inode->i_sb, blknr, 0);
@@ -289,7 +298,7 @@ static inline struct bio *erofs_read_raw_page(
}
/* pa must be block-aligned for raw reading */
- BUG_ON(erofs_blkoff(map.m_pa) != 0);
+ DBG_BUGON(erofs_blkoff(map.m_pa));
/* max # of continuous pages */
if (nblocks > DIV_ROUND_UP(map.m_plen, PAGE_SIZE))
@@ -357,7 +366,7 @@ static int erofs_raw_access_readpage(struct file *file,
struct page *page)
if (IS_ERR(bio))
return PTR_ERR(bio);
- BUG_ON(bio != NULL);/* since we have only one bio -- must be NULL */
+ DBG_BUGON(bio); /* since we have only one bio -- must be NULL */
return 0;
}
@@ -395,7 +404,7 @@ static int erofs_raw_access_readpages(struct file *filp,
/* pages could still be locked */
put_page(page);
}
- BUG_ON(!list_empty(pages));
+ DBG_BUGON(!list_empty(pages));
/* the rare case (end in gaps) */
if (unlikely(bio != NULL))
--
1.9.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 2/6] staging: erofs: code cleanup for option parsing of fault_injection
Hi Chengguang,
On 2018/9/17 23:34, Chengguang Xu wrote:
> Define a dummpy function of erofs_build_fault_attr() when macro
> CONFIG_EROFS_FAULT_INJECTION is disabled, so that we don't have to
> check the macro in calling place. Based on above adjustment,
> do proper code cleanup for option parsing of fault_injection.
>
> Signed-off-by: Chengguang Xu
> ---
> drivers/staging/erofs/super.c | 33 -
> 1 file changed, 20 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/staging/erofs/super.c b/drivers/staging/erofs/super.c
> index 9e421536cbdf..7ce2fd3d49f3 100644
> --- a/drivers/staging/erofs/super.c
> +++ b/drivers/staging/erofs/super.c
> @@ -145,10 +145,14 @@ char *erofs_fault_name[FAULT_MAX] = {
> [FAULT_KMALLOC] = "kmalloc",
> };
>
> -static void erofs_build_fault_attr(struct erofs_sb_info *sbi,
> - unsigned int rate)
> +static int erofs_build_fault_attr(struct erofs_sb_info *sbi,
> + substring_t *args)
> {
> struct erofs_fault_info *ffi = &sbi->fault_info;
> + int rate = 0;
> +
> + if (args->from && match_int(args, &rate))
> + return -EINVAL;
>
> if (rate) {
> atomic_set(&ffi->inject_ops, 0);
> @@ -157,6 +161,15 @@ static void erofs_build_fault_attr(struct erofs_sb_info
> *sbi,
> } else {
> memset(ffi, 0, sizeof(struct erofs_fault_info));
> }
> +
> + set_opt(sbi, FAILt_INJECTION);
drivers/staging/erofs/super.c: In function ‘__erofs_build_fault_attr’:
drivers/staging/erofs/internal.h:176:51: error: ‘EROFS_MOUNT_FAILt_INJECTION’
undeclared (first use in this function)
#define set_opt(sbi, option) ((sbi)->mount_opt |= EROFS_MOUNT_##option)
^
drivers/staging/erofs/super.c:166:2: note: in expansion of macro ‘set_opt’
set_opt(sbi, FAILt_INJECTION);
^
drivers/staging/erofs/internal.h:176:51: note: each undeclared identifier is
reported only once for each function it appears in
#define set_opt(sbi, option) ((sbi)->mount_opt |= EROFS_MOUNT_##option)
^
drivers/staging/erofs/super.c:166:2: note: in expansion of macro ‘set_opt’
set_opt(sbi, FAILt_INJECTION);
^
Thanks,
Gao Xiang
> +}
> +#else
> +static int erofs_build_fault_attr(struct erofs_sb_info *sbi,
> + substring_t *args)
> +{
> + infoln("fault_injection options not supported");
> + return 0;
> }
> #endif
>
> @@ -193,7 +206,7 @@ static int parse_options(struct super_block *sb, char
> *options)
> {
> substring_t args[MAX_OPT_ARGS];
> char *p;
> - int arg = 0;
> + int err;
>
> if (!options)
> return 0;
> @@ -238,18 +251,12 @@ static int parse_options(struct super_block *sb, char
> *options)
> infoln("noacl options not supported");
> break;
> #endif
> -#ifdef CONFIG_EROFS_FAULT_INJECTION
> - case Opt_fault_injection:
> - if (args->from && match_int(args, &arg))
> - return -EINVAL;
> - erofs_build_fault_attr(EROFS_SB(sb), arg);
> - set_opt(EROFS_SB(sb), FAULT_INJECTION);
> - break;
> -#else
> case Opt_fault_injection:
> - infoln("fault_injection options not supported");
> + err = erofs_build_fault_attr(EROFS_SB(sb), args);
> + if (err)
> + return err;
> break;
> -#endif
> +
> default:
> errln("Unrecognized mount option \"%s\" "
> "or missing value", p);
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 0/8] staging: erofs: error handing and more tracepoints
Hi Greg, On 2018/9/18 19:19, Greg Kroah-Hartman wrote: > On Fri, Sep 14, 2018 at 10:40:22PM +0800, Gao Xiang wrote: >> In order to avoid conflicts with cleanup patches, Chao and I think >> it is better to send reviewed preview patches in the erofs mailing list >> to the community in time. >> >> So here is reviewed & tested patches right now, which clean up and >> enhance the error handing and add some tracepoints for decompression. >> >> Note that in this patchset, bare use of 'unsigned' and NULL comparison are >> also fixed compared with the preview patches according to the previous >> discussion in the staging mailing list. > > I applied this, but I need to go delete it as this patch series adds a > build warning to the system: > > In file included from drivers/staging/erofs/unzip_vle.h:16:0, > from drivers/staging/erofs/unzip_vle.c:13: > drivers/staging/erofs/unzip_vle.c: In function ‘z_erofs_map_blocks_iter’: > drivers/staging/erofs/internal.h:303:34: warning: ‘pblk’ may be used > uninitialized in this function [-Wmaybe-uninitialized] > #define blknr_to_addr(nr) ((erofs_off_t)(nr) * EROFS_BLKSIZ) > ^ > drivers/staging/erofs/unzip_vle.c:1574:20: note: ‘pblk’ was declared here > erofs_blk_t mblk, pblk; > ^~~~ > > Please fix that up and resend. strange... my compiler (4.8.4) and huawei internal CI don't report that, and this patchset has been in Chao's tree for a while, I don't get any report so far... I just looked into that code again and it seems a false warning since 1) this code is heavily running on the products and working fine till now. 2) pblk gets a proper value before unzip_vle.c:1690 map->m_pa = blknr_to_addr(pblk); so I think I need to silence this warning for now and check if there is a really issue Thanks, Gao Xiang > > thanks, > > greg k-h > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 0/8] staging: erofs: error handing and more tracepoints
Hi Greg,
On 2018/9/18 20:14, Greg Kroah-Hartman wrote:
> On Tue, Sep 18, 2018 at 08:02:30PM +0800, Gao Xiang wrote:
>> Hi Greg,
>>
>> On 2018/9/18 19:19, Greg Kroah-Hartman wrote:
>>> On Fri, Sep 14, 2018 at 10:40:22PM +0800, Gao Xiang wrote:
>>>> In order to avoid conflicts with cleanup patches, Chao and I think
>>>> it is better to send reviewed preview patches in the erofs mailing list
>>>> to the community in time.
>>>>
>>>> So here is reviewed & tested patches right now, which clean up and
>>>> enhance the error handing and add some tracepoints for decompression.
>>>>
>>>> Note that in this patchset, bare use of 'unsigned' and NULL comparison are
>>>> also fixed compared with the preview patches according to the previous
>>>> discussion in the staging mailing list.
>>>
>>> I applied this, but I need to go delete it as this patch series adds a
>>> build warning to the system:
>>>
>>> In file included from drivers/staging/erofs/unzip_vle.h:16:0,
>>> from drivers/staging/erofs/unzip_vle.c:13:
>>> drivers/staging/erofs/unzip_vle.c: In function ‘z_erofs_map_blocks_iter’:
>>> drivers/staging/erofs/internal.h:303:34: warning: ‘pblk’ may be used
>>> uninitialized in this function [-Wmaybe-uninitialized]
>>> #define blknr_to_addr(nr) ((erofs_off_t)(nr) * EROFS_BLKSIZ)
>>> ^
>>> drivers/staging/erofs/unzip_vle.c:1574:20: note: ‘pblk’ was declared here
>>> erofs_blk_t mblk, pblk;
>>> ^~~~
>>>
>>> Please fix that up and resend.
>>
>> strange... my compiler (4.8.4) and huawei internal CI don't report that,
>> and this patchset has been in Chao's tree for a while, I don't get any
>> report so far...
>>
>> I just looked into that code again and it seems a false warning since
>> 1) this code is heavily running on the products and working fine till now.
>> 2) pblk gets a proper value before unzip_vle.c:1690 map->m_pa =
>> blknr_to_addr(pblk);
>>
>> so I think I need to silence this warning for now and check if there is a
>> really issue
>
> Don't just silent the warning. Usually gcc now properly detects where
> there really is a problem, it should not be a false-positive. And in
> looking at the code, it does seem that pblk could not be set if one of
> the different case statements is taken and then exact_hitted: is jumped
> to, right?
pblk is assigned before each "goto exact_hitted" and "break;" here.
1641 switch (cluster_type) {
1642 case Z_EROFS_VLE_CLUSTER_TYPE_PLAIN:
1643 if (ofs_rem >= logical_cluster_ofs)
1644 map->m_flags ^= EROFS_MAP_ZIPPED;
1645 /* fallthrough */
1646 case Z_EROFS_VLE_CLUSTER_TYPE_HEAD:
1647 if (ofs_rem == logical_cluster_ofs) {
1648 pblk = le32_to_cpu(di->di_u.blkaddr);
1649 goto exact_hitted; <--- assigned
1650 }
1651
1652 if (ofs_rem > logical_cluster_ofs) {
1653 ofs = (u64)lcn * clustersize | logical_cluster_ofs;
1654 pblk = le32_to_cpu(di->di_u.blkaddr);
1655 break; <--- assigned
1656 }
1657
1658 /* logical cluster number should be >= 1 */
1659 if (unlikely(!lcn)) {
1660 errln("invalid logical cluster 0 at nid %llu",
1661 EROFS_V(inode)->nid);
1662 err = -EIO;
1663 goto unmap_out; <--- bypass no need it
1664 }
1665 end = ((u64)lcn-- * clustersize) | logical_cluster_ofs;
1666 /* fallthrough */
1667 case Z_EROFS_VLE_CLUSTER_TYPE_NONHEAD:
1668 /* get the correspoinding first chunk */
1669 err = vle_get_logical_extent_head(&ctx, lcn, &ofs,
1670 &pblk, &map->m_flags);
1671 mpage = *mpage_ret;
1672
1673 if (unlikely(err)) {
1674 if (mpage)
1675 goto unmap_out; <--- bypass no need it
1676 goto out; <--- bypass no need it
1677 }
1678 break; <-- pblk should be assigned in
vle_get_logical_extent_head
if no error occurred in
vle_get_logical_extent_head
1679 default:
1680
Re: [PATCH 0/8] staging: erofs: error handing and more tracepoints
On 2018/9/18 21:09, Greg Kroah-Hartman wrote: > On Tue, Sep 18, 2018 at 04:03:37PM +0300, Dan Carpenter wrote: >> On Tue, Sep 18, 2018 at 08:31:22PM +0800, Gao Xiang wrote: >>> (I have no clang environment to build kernel yet... :( I will try later, >>> but I have no idea why it happens, >>> and it seems a false warning). >>> >>> Thanks, >>> Gao Xiang >>> >>>> >>>> Or is gcc just being "dumb" here? What about clang, have you tried >>>> building it with that compiler as well? >> >> Yeah. Gcc is wrong. Gcc used to be extra conservative and missed a >> bunch of bugs, but I guess now they're going in to opposite direction? >> >> Smatch gets this correct, but you have to rebuild the cross function DB >> twice to make the warning go away. It could be because it starts out >> with the old vle_get_logical_extent_head() information so it thinks it >> knows how that function works. Hi Dan, Thanks for taking time to confirm this and the detailed explanation ;) > > Ok, thanks for checking (both of you). Just initialize the variable to > keep gcc from printing foolish warnings. Hi Greg, Ok, if you don't tend to use `uninitialized_var(...)', I will initialize a dumb value...but I think it is really useless to initialize it... :( I will resend the related false warning patch independently soon... Thanks, Gao Xiang > > thanks, > > greg k-h > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: erofs: initialize `pblk' with 0 first in `z_erofs_map_blocks_iter'
This commit message helps to understand why `pblk' is assigned with 0 here. [ Greg reported a warning raised by gcc. ] In file included from drivers/staging/erofs/unzip_vle.h:16:0, from drivers/staging/erofs/unzip_vle.c:13: drivers/staging/erofs/unzip_vle.c: In function ‘z_erofs_map_blocks_iter’: drivers/staging/erofs/internal.h:303:34: warning: ‘pblk’ may be used uninitialized in this function [-Wmaybe-uninitialized] #define blknr_to_addr(nr) ((erofs_off_t)(nr) * EROFS_BLKSIZ) ^ drivers/staging/erofs/unzip_vle.c:1574:20: note: ‘pblk’ was declared here erofs_blk_t mblk, pblk; ^~~~ Actually, it is a false-positive warning when looking into that. [1] Just initialize the variable to keep gcc from printing foolish warnings. [1] https://lists.ozlabs.org/pipermail/linux-erofs/2018-September/000637.html Reported-by: Greg Kroah-Hartman Thanks-to: Dan Carpenter Signed-off-by: Gao Xiang --- drivers/staging/erofs/unzip_vle.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/erofs/unzip_vle.c b/drivers/staging/erofs/unzip_vle.c index ad3b7bb..3ff8adf 100644 --- a/drivers/staging/erofs/unzip_vle.c +++ b/drivers/staging/erofs/unzip_vle.c @@ -1571,7 +1571,7 @@ int z_erofs_map_blocks_iter(struct inode *inode, unsigned int lcn; u32 ofs_rem; - erofs_blk_t mblk, pblk; + erofs_blk_t mblk, pblk = 0; struct page *mpage = *mpage_ret; struct z_erofs_vle_decompressed_index *di; unsigned int cluster_type, logical_cluster_ofs; -- 1.9.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 1/6] staging: erofs: code cleanup for erofs_kmalloc()
On 2018/9/18 18:41, Chao Yu wrote: > On 2018/9/17 23:34, Chengguang Xu wrote: >> Define a dummy function of time_to_inject()/erofs_show_injection_info(), >> so that we don't have to check macro CONFIG_EROFS_FAULT_INJECTION in >> calling place. >> >> Signed-off-by: Chengguang Xu > > Reviewed-by: Chao Yu > Reviewed-by: Gao Xiang Thanks, Gao Xiang > Thanks, > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 2/6] staging: erofs: code cleanup for option parsing of fault_injection
Hi Chengguang,
On 2018/9/18 18:47, cgxu519 wrote:
> On 09/18/2018 03:07 PM, Gao Xiang wrote:
>> Hi Chengguang,
>>
>> On 2018/9/17 23:34, Chengguang Xu wrote:
>>> Define a dummpy function of erofs_build_fault_attr() when macro
>>> CONFIG_EROFS_FAULT_INJECTION is disabled, so that we don't have to
>>> check the macro in calling place. Based on above adjustment,
>>> do proper code cleanup for option parsing of fault_injection.
>>>
>>> Signed-off-by: Chengguang Xu
>>> ---
>>> drivers/staging/erofs/super.c | 33 -
>>> 1 file changed, 20 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/staging/erofs/super.c b/drivers/staging/erofs/super.c
>>> index 9e421536cbdf..7ce2fd3d49f3 100644
>>> --- a/drivers/staging/erofs/super.c
>>> +++ b/drivers/staging/erofs/super.c
>>> @@ -145,10 +145,14 @@ char *erofs_fault_name[FAULT_MAX] = {
>>> [FAULT_KMALLOC] = "kmalloc",
>>> };
>>> -static void erofs_build_fault_attr(struct erofs_sb_info *sbi,
>>> - unsigned int rate)
>>> +static int erofs_build_fault_attr(struct erofs_sb_info *sbi,
>>> + substring_t *args)
>>> {
>>> struct erofs_fault_info *ffi = &sbi->fault_info;
>>> + int rate = 0;
>>> +
>>> + if (args->from && match_int(args, &rate))
>>> + return -EINVAL;
>>> if (rate) {
>>> atomic_set(&ffi->inject_ops, 0);
>>> @@ -157,6 +161,15 @@ static void erofs_build_fault_attr(struct
>>> erofs_sb_info *sbi,
>>> } else {
>>> memset(ffi, 0, sizeof(struct erofs_fault_info));
>>> }
>>> +
>>> + set_opt(sbi, FAILt_INJECTION);
>> drivers/staging/erofs/super.c: In function ‘__erofs_build_fault_attr’:
>> drivers/staging/erofs/internal.h:176:51: error:
>> ‘EROFS_MOUNT_FAILt_INJECTION’ undeclared (first use in this function)
>> #define set_opt(sbi, option) ((sbi)->mount_opt |= EROFS_MOUNT_##option)
>> ^
>> drivers/staging/erofs/super.c:166:2: note: in expansion of macro ‘set_opt’
>> set_opt(sbi, FAILt_INJECTION);
>> ^
>> drivers/staging/erofs/internal.h:176:51: note: each undeclared identifier is
>> reported only once for each function it appears in
>> #define set_opt(sbi, option) ((sbi)->mount_opt |= EROFS_MOUNT_##option)
>> ^
>> drivers/staging/erofs/super.c:166:2: note: in expansion of macro ‘set_opt’
>> set_opt(sbi, FAILt_INJECTION);
>> ^
>
> Hi Xiang,
>
> I'm really sorry for that, I'm curious how it passed my building test.
> I deleted all existing config and binary files and tested with/without
> INJECTION config this time.
I have no idea either...
No worry about that, just resend your fixed patch.
Thanks,
Gao Xiang
>
>
> Thanks,
> Chengguang
>
>
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: erofs: initialize `pblk' with 0 first in `z_erofs_map_blocks_iter'
Hi Greg, On 2018/9/18 21:57, Greg Kroah-Hartman wrote: > On Tue, Sep 18, 2018 at 09:44:36PM +0800, Gao Xiang wrote: >> This commit message helps to understand why `pblk' is assigned with 0 here. >> >> [ Greg reported a warning raised by gcc. ] >> In file included from drivers/staging/erofs/unzip_vle.h:16:0, >> from drivers/staging/erofs/unzip_vle.c:13: >> drivers/staging/erofs/unzip_vle.c: In function ‘z_erofs_map_blocks_iter’: >> drivers/staging/erofs/internal.h:303:34: warning: ‘pblk’ may be used >> uninitialized in this function [-Wmaybe-uninitialized] >> #define blknr_to_addr(nr) ((erofs_off_t)(nr) * EROFS_BLKSIZ) >> ^ >> drivers/staging/erofs/unzip_vle.c:1574:20: note: ‘pblk’ was declared here >> erofs_blk_t mblk, pblk; >> ^~~~ >> >> Actually, it is a false-positive warning when looking into that. [1] >> Just initialize the variable to keep gcc from printing foolish warnings. >> >> [1] https://lists.ozlabs.org/pipermail/linux-erofs/2018-September/000637.html >> >> Reported-by: Greg Kroah-Hartman >> Thanks-to: Dan Carpenter >> Signed-off-by: Gao Xiang >> --- >> drivers/staging/erofs/unzip_vle.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) > > Please roll this into the original patch. I have dropped that whole > series from my tree, it is no longer present and needs to be resent in > order for me to be able to accept any of these. OK, will send. it's my honor. Thanks, Gao Xiang > > thanks, > > greg k-h > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 1/8] staging: erofs: fix a missing endian conversion
This patch fixes a missing endian conversion in
vle_get_logical_extent_head.
Reviewed-by: Chao Yu
Signed-off-by: Gao Xiang
---
drivers/staging/erofs/unzip_vle.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/erofs/unzip_vle.c
b/drivers/staging/erofs/unzip_vle.c
index 21874b7..d16e3dc 100644
--- a/drivers/staging/erofs/unzip_vle.c
+++ b/drivers/staging/erofs/unzip_vle.c
@@ -1488,6 +1488,7 @@ static erofs_off_t vle_get_logical_extent_head(
struct super_block *const sb = inode->i_sb;
const unsigned int clusterbits = EROFS_SB(sb)->clusterbits;
const unsigned int clustersize = 1 << clusterbits;
+ unsigned int delta0;
if (page->index != blkaddr) {
kunmap_atomic(*kaddr_iter);
@@ -1502,12 +1503,13 @@ static erofs_off_t vle_get_logical_extent_head(
di = *kaddr_iter + vle_extent_blkoff(inode, lcn);
switch (vle_cluster_type(di)) {
case Z_EROFS_VLE_CLUSTER_TYPE_NONHEAD:
- BUG_ON(!di->di_u.delta[0]);
- BUG_ON(lcn < di->di_u.delta[0]);
+ delta0 = le16_to_cpu(di->di_u.delta[0]);
+ DBG_BUGON(!delta0);
+ DBG_BUGON(lcn < delta0);
ofs = vle_get_logical_extent_head(inode,
page_iter, kaddr_iter,
- lcn - di->di_u.delta[0], pcn, flags);
+ lcn - delta0, pcn, flags);
break;
case Z_EROFS_VLE_CLUSTER_TYPE_PLAIN:
*flags ^= EROFS_MAP_ZIPPED;
--
1.9.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 2/8] staging: erofs: clean up z_erofs_map_blocks_iter
This patch mainly introduces `vle_map_blocks_iter_ctx' to clean up
z_erofs_map_blocks_iter and vle_get_logical_extent_head.
It changes the return value of `vle_get_logical_extent_head' to int
for the later error handing. In addition, it also renames `pcn' to
`pblk' since only `pblk' exists in erofs compression ondisk format.
Reviewed-by: Chao Yu
Signed-off-by: Gao Xiang
---
drivers/staging/erofs/unzip_vle.c | 140 +-
1 file changed, 78 insertions(+), 62 deletions(-)
diff --git a/drivers/staging/erofs/unzip_vle.c
b/drivers/staging/erofs/unzip_vle.c
index d16e3dc..1e89ff6 100644
--- a/drivers/staging/erofs/unzip_vle.c
+++ b/drivers/staging/erofs/unzip_vle.c
@@ -1410,6 +1410,13 @@ static int z_erofs_vle_normalaccess_readpages(
.readpages = z_erofs_vle_normalaccess_readpages,
};
+/*
+ * Variable-sized Logical Extent (Fixed Physical Cluster) Compression Mode
+ * ---
+ * VLE compression mode attempts to compress a number of logical data into
+ * a physical cluster with a fixed size.
+ * VLE compression mode uses "struct z_erofs_vle_decompressed_index".
+ */
#define __vle_cluster_advise(x, bit, bits) \
((le16_to_cpu(x) >> (bit)) & ((1 << (bits)) - 1))
@@ -1465,90 +1472,96 @@ static int z_erofs_vle_normalaccess_readpages(
return erofs_blkoff(iloc(sbi, vi->nid) + ofs);
}
-/*
- * Variable-sized Logical Extent (Fixed Physical Cluster) Compression Mode
- * ---
- * VLE compression mode attempts to compress a number of logical data into
- * a physical cluster with a fixed size.
- * VLE compression mode uses "struct z_erofs_vle_decompressed_index".
- */
-static erofs_off_t vle_get_logical_extent_head(
- struct inode *inode,
- struct page **page_iter,
- void **kaddr_iter,
- unsigned int lcn, /* logical cluster number */
- erofs_blk_t *pcn,
- unsigned int *flags)
+struct vle_map_blocks_iter_ctx {
+ struct inode *inode;
+ struct super_block *sb;
+ unsigned int clusterbits;
+
+ struct page **mpage_ret;
+ void **kaddr_ret;
+};
+
+static int
+vle_get_logical_extent_head(const struct vle_map_blocks_iter_ctx *ctx,
+ unsigned int lcn, /* logical cluster number */
+ unsigned long long *ofs,
+ erofs_blk_t *pblk,
+ unsigned int *flags)
{
- /* for extent meta */
- struct page *page = *page_iter;
- erofs_blk_t blkaddr = vle_extent_blkaddr(inode, lcn);
+ const unsigned int clustersize = 1 << ctx->clusterbits;
+ const erofs_blk_t mblk = vle_extent_blkaddr(ctx->inode, lcn);
+ struct page *mpage = *ctx->mpage_ret; /* extent metapage */
+
struct z_erofs_vle_decompressed_index *di;
- unsigned long long ofs;
- struct super_block *const sb = inode->i_sb;
- const unsigned int clusterbits = EROFS_SB(sb)->clusterbits;
- const unsigned int clustersize = 1 << clusterbits;
- unsigned int delta0;
-
- if (page->index != blkaddr) {
- kunmap_atomic(*kaddr_iter);
- unlock_page(page);
- put_page(page);
+ unsigned int cluster_type, delta0;
- page = erofs_get_meta_page_nofail(sb, blkaddr, false);
- *page_iter = page;
- *kaddr_iter = kmap_atomic(page);
+ if (mpage->index != mblk) {
+ kunmap_atomic(*ctx->kaddr_ret);
+ unlock_page(mpage);
+ put_page(mpage);
+
+ mpage = erofs_get_meta_page_nofail(ctx->sb, mblk, false);
+ *ctx->mpage_ret = mpage;
+ *ctx->kaddr_ret = kmap_atomic(mpage);
}
- di = *kaddr_iter + vle_extent_blkoff(inode, lcn);
- switch (vle_cluster_type(di)) {
+ di = *ctx->kaddr_ret + vle_extent_blkoff(ctx->inode, lcn);
+
+ cluster_type = vle_cluster_type(di);
+ switch (cluster_type) {
case Z_EROFS_VLE_CLUSTER_TYPE_NONHEAD:
delta0 = le16_to_cpu(di->di_u.delta[0]);
DBG_BUGON(!delta0);
DBG_BUGON(lcn < delta0);
- ofs = vle_get_logical_extent_head(inode,
- page_iter, kaddr_iter,
- lcn - delta0, pcn, flags);
- break;
+ return vle_get_logical_extent_head(ctx,
+ lcn - delta0, ofs, pblk, flags);
case Z_EROFS_VLE_CLUSTER_TYPE_PLAIN:
*flags ^= EROFS_MAP_ZIPPED;
+ /* fallthrough */
case Z_EROFS_VLE_CLUSTER_TYPE_HEAD:
/* clustersize should be a power of two */
- ofs = ((u64)lcn << clusterbits) +
+ *ofs = ((u64)lcn << ctx->clusterbits) +
(le16_to_cpu(di->di_clusterofs) & (clustersize - 1));
-
[PATCH v2 0/8] staging: erofs: error handing and more tracepoints
change log v2: - fix a gcc warning reported by Greg for the patch staging: erofs: clean up z_erofs_map_blocks_iter In order to avoid conflicts with cleanup patches, Chao and I think it is better to send reviewed preview patches in the erofs mailing list to the community in time. So here is reviewed & tested patches right now, which clean up and enhance the error handing and add some tracepoints for decompression. Note that in this patchset, bare use of 'unsigned' and NULL comparison are also fixed compared with the preview patches according to the previous discussion in the staging mailing list. Thanks, Gao Xiang Chen Gong (2): staging: erofs: add trace points for reading zipped data staging: erofs: replace BUG_ON with DBG_BUGON in data.c Gao Xiang (6): staging: erofs: fix a missing endian conversion staging: erofs: clean up z_erofs_map_blocks_iter staging: erofs: complete error handing of z_erofs_map_blocks_iter staging: erofs: fix a bug when appling cache strategy staging: erofs: complete error handing of z_erofs_do_read_page staging: erofs: avoid magic constants when initializing clusterbits drivers/staging/erofs/data.c | 31 ++-- drivers/staging/erofs/include/trace/events/erofs.h | 20 ++- drivers/staging/erofs/super.c | 5 +- drivers/staging/erofs/unzip_vle.c | 196 + 4 files changed, 163 insertions(+), 89 deletions(-) -- 1.9.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 3/8] staging: erofs: complete error handing of z_erofs_map_blocks_iter
This patch completes error handing of z_erofs_map_blocks_iter
and vle_get_logical_extent_head, including no memory and
io error cases.
Reviewed-by: Chao Yu
Signed-off-by: Gao Xiang
---
drivers/staging/erofs/unzip_vle.c | 35 ++-
1 file changed, 26 insertions(+), 9 deletions(-)
diff --git a/drivers/staging/erofs/unzip_vle.c
b/drivers/staging/erofs/unzip_vle.c
index 1e89ff6..94c35b8 100644
--- a/drivers/staging/erofs/unzip_vle.c
+++ b/drivers/staging/erofs/unzip_vle.c
@@ -1500,7 +1500,11 @@ struct vle_map_blocks_iter_ctx {
unlock_page(mpage);
put_page(mpage);
- mpage = erofs_get_meta_page_nofail(ctx->sb, mblk, false);
+ mpage = erofs_get_meta_page(ctx->sb, mblk, false);
+ if (IS_ERR(mpage)) {
+ *ctx->mpage_ret = NULL;
+ return PTR_ERR(mpage);
+ }
*ctx->mpage_ret = mpage;
*ctx->kaddr_ret = kmap_atomic(mpage);
}
@@ -1511,9 +1515,12 @@ struct vle_map_blocks_iter_ctx {
switch (cluster_type) {
case Z_EROFS_VLE_CLUSTER_TYPE_NONHEAD:
delta0 = le16_to_cpu(di->di_u.delta[0]);
- DBG_BUGON(!delta0);
- DBG_BUGON(lcn < delta0);
-
+ if (unlikely(!delta0 || delta0 > lcn)) {
+ errln("invalid NONHEAD dl0 %u at lcn %u of nid %llu",
+ delta0, lcn, EROFS_V(ctx->inode)->nid);
+ DBG_BUGON(1);
+ return -EIO;
+ }
return vle_get_logical_extent_head(ctx,
lcn - delta0, ofs, pblk, flags);
case Z_EROFS_VLE_CLUSTER_TYPE_PLAIN:
@@ -1526,7 +1533,10 @@ struct vle_map_blocks_iter_ctx {
*pblk = le32_to_cpu(di->di_u.blkaddr);
break;
default:
- BUG_ON(1);
+ errln("unknown cluster type %u at lcn %u of nid %llu",
+ cluster_type, lcn, EROFS_V(ctx->inode)->nid);
+ DBG_BUGON(1);
+ return -EIO;
}
return 0;
}
@@ -1583,7 +1593,11 @@ int z_erofs_map_blocks_iter(struct inode *inode,
if (mpage != NULL)
put_page(mpage);
- mpage = erofs_get_meta_page_nofail(ctx.sb, mblk, false);
+ mpage = erofs_get_meta_page(ctx.sb, mblk, false);
+ if (IS_ERR(mpage)) {
+ err = PTR_ERR(mpage);
+ goto out;
+ }
*mpage_ret = mpage;
} else {
lock_page(mpage);
@@ -1646,8 +1660,11 @@ int z_erofs_map_blocks_iter(struct inode *inode,
&pblk, &map->m_flags);
mpage = *mpage_ret;
- if (unlikely(err))
- goto unmap_out;
+ if (unlikely(err)) {
+ if (mpage)
+ goto unmap_out;
+ goto out;
+ }
break;
default:
errln("unknown cluster type %u at offset %llu of nid %llu",
@@ -1671,7 +1688,7 @@ int z_erofs_map_blocks_iter(struct inode *inode,
map->m_llen, map->m_plen, map->m_flags);
/* aggressively BUG_ON iff CONFIG_EROFS_FS_DEBUG is on */
- DBG_BUGON(err < 0);
+ DBG_BUGON(err < 0 && err != -ENOMEM);
return err;
}
--
1.9.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 4/8] staging: erofs: fix a bug when appling cache strategy
As described in Kconfig, the last compressed pack should be cached
for further reading for either `EROFS_FS_ZIP_CACHE_UNIPOLAR' or
`EROFS_FS_ZIP_CACHE_BIPOLAR' by design.
However, there is a bug in z_erofs_do_read_page, it will
switch `initial' to `false' at the very beginning before it decides
to cache the last compressed pack.
caching strategy should work properly after appling this patch.
Reviewed-by: Chao Yu
Signed-off-by: Gao Xiang
---
drivers/staging/erofs/unzip_vle.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/erofs/unzip_vle.c
b/drivers/staging/erofs/unzip_vle.c
index 94c35b8..3296538 100644
--- a/drivers/staging/erofs/unzip_vle.c
+++ b/drivers/staging/erofs/unzip_vle.c
@@ -629,7 +629,7 @@ static int z_erofs_do_read_page(struct z_erofs_vle_frontend
*fe,
/* go ahead the next map_blocks */
debugln("%s: [out-of-range] pos %llu", __func__, offset + cur);
- if (!z_erofs_vle_work_iter_end(builder))
+ if (z_erofs_vle_work_iter_end(builder))
fe->initial = false;
map->m_la = offset + cur;
--
1.9.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 6/8] staging: erofs: avoid magic constants when initializing clusterbits
Currently erofs only supports clustersize == blocksize.
and clustersize == 2^n * blocksize will be supported in the future.
Reviewed-by: Chao Yu
Signed-off-by: Gao Xiang
---
drivers/staging/erofs/super.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/erofs/super.c b/drivers/staging/erofs/super.c
index 2109b03..9e42153 100644
--- a/drivers/staging/erofs/super.c
+++ b/drivers/staging/erofs/super.c
@@ -116,9 +116,10 @@ static int superblock_read(struct super_block *sb)
#endif
sbi->islotbits = ffs(sizeof(struct erofs_inode_v1)) - 1;
#ifdef CONFIG_EROFS_FS_ZIP
- sbi->clusterbits = 12;
+ /* TODO: clusterbits should be related to inode */
+ sbi->clusterbits = blkszbits;
- if (1 << (sbi->clusterbits - 12) > Z_EROFS_CLUSTER_MAX_PAGES)
+ if (1 << (sbi->clusterbits - PAGE_SHIFT) > Z_EROFS_CLUSTER_MAX_PAGES)
errln("clusterbits %u is not supported on this kernel",
sbi->clusterbits);
#endif
--
1.9.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 5/8] staging: erofs: complete error handing of z_erofs_do_read_page
This patch completes error handing code of z_erofs_do_read_page.
PG_error will be set when some read error happens, therefore
z_erofs_onlinepage_endio will unlock this page without setting
PG_uptodate.
Reviewed-by: Chao Yu
Signed-off-by: Gao Xiang
---
drivers/staging/erofs/unzip_vle.c | 20 +++-
1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/drivers/staging/erofs/unzip_vle.c
b/drivers/staging/erofs/unzip_vle.c
index 3296538..08c424c 100644
--- a/drivers/staging/erofs/unzip_vle.c
+++ b/drivers/staging/erofs/unzip_vle.c
@@ -611,7 +611,7 @@ static int z_erofs_do_read_page(struct z_erofs_vle_frontend
*fe,
enum z_erofs_page_type page_type;
unsigned int cur, end, spiltted, index;
- int err;
+ int err = 0;
/* register locked file pages as online pages in pack */
z_erofs_onlinepage_init(page);
@@ -638,12 +638,11 @@ static int z_erofs_do_read_page(struct
z_erofs_vle_frontend *fe,
if (unlikely(err))
goto err_out;
- /* deal with hole (FIXME! broken now) */
if (unlikely(!(map->m_flags & EROFS_MAP_MAPPED)))
goto hitted;
DBG_BUGON(map->m_plen != 1 << sbi->clusterbits);
- BUG_ON(erofs_blkoff(map->m_pa));
+ DBG_BUGON(erofs_blkoff(map->m_pa));
err = z_erofs_vle_work_iter_begin(builder, sb, map, &fe->owned_head);
if (unlikely(err))
@@ -688,7 +687,7 @@ static int z_erofs_do_read_page(struct z_erofs_vle_frontend
*fe,
err = z_erofs_vle_work_add_page(builder,
newpage, Z_EROFS_PAGE_TYPE_EXCLUSIVE);
- if (!err)
+ if (likely(!err))
goto retry;
}
@@ -699,9 +698,10 @@ static int z_erofs_do_read_page(struct
z_erofs_vle_frontend *fe,
/* FIXME! avoid the last relundant fixup & endio */
z_erofs_onlinepage_fixup(page, index, true);
- ++spiltted;
- /* also update nr_pages and increase queued_pages */
+ /* bump up the number of spiltted parts of a page */
+ ++spiltted;
+ /* also update nr_pages */
work->nr_pages = max_t(pgoff_t, work->nr_pages, index + 1);
next_part:
/* can be used for verification */
@@ -711,16 +711,18 @@ static int z_erofs_do_read_page(struct
z_erofs_vle_frontend *fe,
if (end > 0)
goto repeat;
+out:
/* FIXME! avoid the last relundant fixup & endio */
z_erofs_onlinepage_endio(page);
debugln("%s, finish page: %pK spiltted: %u map->m_llen %llu",
__func__, page, spiltted, map->m_llen);
- return 0;
+ return err;
+ /* if some error occurred while processing this page */
err_out:
- /* TODO: the missing error handing cases */
- return err;
+ SetPageError(page);
+ goto out;
}
static void z_erofs_vle_unzip_kickoff(void *ptr, int bios)
--
1.9.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 7/8] staging: erofs: add trace points for reading zipped data
From: Chen Gong
This patch adds trace points for reading zipped data.
Signed-off-by: Chen Gong
Reviewed-by: Chao Yu
Reviewed-by: Gao Xiang
Signed-off-by: Gao Xiang
---
drivers/staging/erofs/include/trace/events/erofs.h | 20 ++--
drivers/staging/erofs/unzip_vle.c | 11 +++
2 files changed, 29 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/erofs/include/trace/events/erofs.h
b/drivers/staging/erofs/include/trace/events/erofs.h
index 5aead93..660c92f 100644
--- a/drivers/staging/erofs/include/trace/events/erofs.h
+++ b/drivers/staging/erofs/include/trace/events/erofs.h
@@ -162,7 +162,8 @@
TP_printk("dev = (%d,%d), nid = %llu, la %llu llen %llu flags %s",
show_dev_nid(__entry),
- __entry->la, __entry->llen, show_map_flags(__entry->flags))
+ __entry->la, __entry->llen,
+ __entry->flags ? show_map_flags(__entry->flags) : "NULL")
);
DEFINE_EVENT(erofs__map_blocks_enter, erofs_map_blocks_flatmode_enter,
@@ -172,6 +173,13 @@
TP_ARGS(inode, map, flags)
);
+DEFINE_EVENT(erofs__map_blocks_enter, z_erofs_map_blocks_iter_enter,
+ TP_PROTO(struct inode *inode, struct erofs_map_blocks *map,
+unsigned int flags),
+
+ TP_ARGS(inode, map, flags)
+);
+
DECLARE_EVENT_CLASS(erofs__map_blocks_exit,
TP_PROTO(struct inode *inode, struct erofs_map_blocks *map,
unsigned int flags, int ret),
@@ -204,7 +212,8 @@
TP_printk("dev = (%d,%d), nid = %llu, flags %s "
"la %llu pa %llu llen %llu plen %llu mflags %s ret %d",
- show_dev_nid(__entry), show_map_flags(__entry->flags),
+ show_dev_nid(__entry),
+ __entry->flags ? show_map_flags(__entry->flags) : "NULL",
__entry->la, __entry->pa, __entry->llen, __entry->plen,
show_mflags(__entry->mflags), __entry->ret)
);
@@ -216,6 +225,13 @@
TP_ARGS(inode, map, flags, ret)
);
+DEFINE_EVENT(erofs__map_blocks_exit, z_erofs_map_blocks_iter_exit,
+ TP_PROTO(struct inode *inode, struct erofs_map_blocks *map,
+unsigned int flags, int ret),
+
+ TP_ARGS(inode, map, flags, ret)
+);
+
TRACE_EVENT(erofs_destroy_inode,
TP_PROTO(struct inode *inode),
diff --git a/drivers/staging/erofs/unzip_vle.c
b/drivers/staging/erofs/unzip_vle.c
index 08c424c..2b9b313 100644
--- a/drivers/staging/erofs/unzip_vle.c
+++ b/drivers/staging/erofs/unzip_vle.c
@@ -13,6 +13,8 @@
#include "unzip_vle.h"
#include
+#include
+
static struct workqueue_struct *z_erofs_workqueue __read_mostly;
static struct kmem_cache *z_erofs_workgroup_cachep __read_mostly;
@@ -613,6 +615,8 @@ static int z_erofs_do_read_page(struct z_erofs_vle_frontend
*fe,
unsigned int cur, end, spiltted, index;
int err = 0;
+ trace_erofs_readpage(page, false);
+
/* register locked file pages as online pages in pack */
z_erofs_onlinepage_init(page);
@@ -1348,6 +1352,9 @@ static inline int __z_erofs_vle_normalaccess_readpages(
struct page *head = NULL;
LIST_HEAD(pagepool);
+ trace_erofs_readpages(mapping->host, lru_to_page(pages),
+ nr_pages, false);
+
#if (EROFS_FS_ZIP_CACHE_LVL >= 2)
f.cachedzone_la = (erofs_off_t)lru_to_page(pages)->index << PAGE_SHIFT;
#endif
@@ -1571,6 +1578,8 @@ int z_erofs_map_blocks_iter(struct inode *inode,
unsigned int cluster_type, logical_cluster_ofs;
int err = 0;
+ trace_z_erofs_map_blocks_iter_enter(inode, map, flags);
+
/* when trying to read beyond EOF, leave it unmapped */
if (unlikely(map->m_la >= inode->i_size)) {
DBG_BUGON(!initial);
@@ -1689,6 +1698,8 @@ int z_erofs_map_blocks_iter(struct inode *inode,
__func__, map->m_la, map->m_pa,
map->m_llen, map->m_plen, map->m_flags);
+ trace_z_erofs_map_blocks_iter_exit(inode, map, flags, err);
+
/* aggressively BUG_ON iff CONFIG_EROFS_FS_DEBUG is on */
DBG_BUGON(err < 0 && err != -ENOMEM);
return err;
--
1.9.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 8/8] staging: erofs: replace BUG_ON with DBG_BUGON in data.c
From: Chen Gong
This patch replace BUG_ON with DBG_BUGON in data.c, and add necessary
error handler.
Signed-off-by: Chen Gong
Reviewed-by: Gao Xiang
Reviewed-by: Chao Yu
Signed-off-by: Gao Xiang
---
drivers/staging/erofs/data.c | 31 ---
1 file changed, 20 insertions(+), 11 deletions(-)
diff --git a/drivers/staging/erofs/data.c b/drivers/staging/erofs/data.c
index e191610..6384f73 100644
--- a/drivers/staging/erofs/data.c
+++ b/drivers/staging/erofs/data.c
@@ -25,7 +25,7 @@ static inline void read_endio(struct bio *bio)
struct page *page = bvec->bv_page;
/* page is already locked */
- BUG_ON(PageUptodate(page));
+ DBG_BUGON(PageUptodate(page));
if (unlikely(err))
SetPageError(page);
@@ -110,12 +110,12 @@ static int erofs_map_blocks_flatmode(struct inode *inode,
struct erofs_map_blocks *map,
int flags)
{
+ int err = 0;
erofs_blk_t nblocks, lastblk;
u64 offset = map->m_la;
struct erofs_vnode *vi = EROFS_V(inode);
trace_erofs_map_blocks_flatmode_enter(inode, map, flags);
- BUG_ON(is_inode_layout_compression(inode));
nblocks = DIV_ROUND_UP(inode->i_size, PAGE_SIZE);
lastblk = nblocks - is_inode_layout_inline(inode);
@@ -142,18 +142,27 @@ static int erofs_map_blocks_flatmode(struct inode *inode,
map->m_plen = inode->i_size - offset;
/* inline data should locate in one meta block */
- BUG_ON(erofs_blkoff(map->m_pa) + map->m_plen > PAGE_SIZE);
+ if (erofs_blkoff(map->m_pa) + map->m_plen > PAGE_SIZE) {
+ DBG_BUGON(1);
+ err = -EIO;
+ goto err_out;
+ }
+
map->m_flags |= EROFS_MAP_META;
} else {
errln("internal error @ nid: %llu (size %llu), m_la 0x%llx",
vi->nid, inode->i_size, map->m_la);
- BUG();
+ DBG_BUGON(1);
+ err = -EIO;
+ goto err_out;
}
out:
map->m_llen = map->m_plen;
+
+err_out:
trace_erofs_map_blocks_flatmode_exit(inode, map, flags, 0);
- return 0;
+ return err;
}
#ifdef CONFIG_EROFS_FS_ZIP
@@ -209,7 +218,7 @@ static inline struct bio *erofs_read_raw_page(
erofs_off_t current_block = (erofs_off_t)page->index;
int err;
- BUG_ON(!nblocks);
+ DBG_BUGON(!nblocks);
if (PageUptodate(page)) {
err = 0;
@@ -252,7 +261,7 @@ static inline struct bio *erofs_read_raw_page(
}
/* for RAW access mode, m_plen must be equal to m_llen */
- BUG_ON(map.m_plen != map.m_llen);
+ DBG_BUGON(map.m_plen != map.m_llen);
blknr = erofs_blknr(map.m_pa);
blkoff = erofs_blkoff(map.m_pa);
@@ -262,7 +271,7 @@ static inline struct bio *erofs_read_raw_page(
void *vsrc, *vto;
struct page *ipage;
- BUG_ON(map.m_plen > PAGE_SIZE);
+ DBG_BUGON(map.m_plen > PAGE_SIZE);
ipage = erofs_get_meta_page(inode->i_sb, blknr, 0);
@@ -289,7 +298,7 @@ static inline struct bio *erofs_read_raw_page(
}
/* pa must be block-aligned for raw reading */
- BUG_ON(erofs_blkoff(map.m_pa) != 0);
+ DBG_BUGON(erofs_blkoff(map.m_pa));
/* max # of continuous pages */
if (nblocks > DIV_ROUND_UP(map.m_plen, PAGE_SIZE))
@@ -357,7 +366,7 @@ static int erofs_raw_access_readpage(struct file *file,
struct page *page)
if (IS_ERR(bio))
return PTR_ERR(bio);
- BUG_ON(bio != NULL);/* since we have only one bio -- must be NULL */
+ DBG_BUGON(bio); /* since we have only one bio -- must be NULL */
return 0;
}
@@ -395,7 +404,7 @@ static int erofs_raw_access_readpages(struct file *filp,
/* pages could still be locked */
put_page(page);
}
- BUG_ON(!list_empty(pages));
+ DBG_BUGON(!list_empty(pages));
/* the rare case (end in gaps) */
if (unlikely(bio != NULL))
--
1.9.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 0/8] staging: erofs: error handing and more tracepoints
Hi Greg, On 2018/9/18 22:36, Greg Kroah-Hartman wrote: > On Tue, Sep 18, 2018 at 10:25:32PM +0800, Gao Xiang wrote: >> change log v2: >> - fix a gcc warning reported by Greg for the patch >> staging: erofs: clean up z_erofs_map_blocks_iter >> >> >> In order to avoid conflicts with cleanup patches, Chao and I think >> it is better to send reviewed preview patches in the erofs mailing list >> to the community in time. >> >> So here is reviewed & tested patches right now, which clean up and >> enhance the error handing and add some tracepoints for decompression. >> >> Note that in this patchset, bare use of 'unsigned' and NULL comparison are >> also fixed compared with the preview patches according to the previous >> discussion in the staging mailing list. > > Thanks for the resend, looks good. Thanks for applying... p.s. sorry for just expression... I mean "with my pleasure", sorry about my english... Thanks, Gao Xiang > > greg k-h > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 2/4] staging: erofs: code cleanup for option parsing of fault_injection
Hi Chengguang,
On 2018/9/18 23:10, Chengguang Xu wrote:
> Define a dummpy function of erofs_build_fault_attr() when macro
> CONFIG_EROFS_FAULT_INJECTION is disabled, so that we don't have to
> check the macro in calling place. Based on above adjustment,
> do proper code cleanup for option parsing of fault_injection.
>
> Signed-off-by: Chengguang Xu
> Reviewed-by: Chao Yu
> ---
> drivers/staging/erofs/super.c | 34 +-
> 1 file changed, 21 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/staging/erofs/super.c b/drivers/staging/erofs/super.c
> index 9e421536cbdf..f496e0c1d04d 100644
> --- a/drivers/staging/erofs/super.c
> +++ b/drivers/staging/erofs/super.c
> @@ -145,10 +145,14 @@ char *erofs_fault_name[FAULT_MAX] = {
> [FAULT_KMALLOC] = "kmalloc",
> };
>
> -static void erofs_build_fault_attr(struct erofs_sb_info *sbi,
> - unsigned int rate)
> +static int erofs_build_fault_attr(struct erofs_sb_info *sbi,
> + substring_t *args)
minor... two checkpatch suggestions out there, it is better to fix
them before merging...
CHECK: Alignment should match open parenthesis
#143: FILE: drivers/staging/erofs/super.c:149:
+static int erofs_build_fault_attr(struct erofs_sb_info *sbi,
+ substring_t *args)
CHECK: Alignment should match open parenthesis
#163: FILE: drivers/staging/erofs/super.c:170:
+static int erofs_build_fault_attr(struct erofs_sb_info *sbi,
+ substring_t *args)
and you could add
Reviewed-by: Gao Xiang
Thanks,
Gao Xiang
> {
> struct erofs_fault_info *ffi = &sbi->fault_info;
> + int rate = 0;
> +
> + if (args->from && match_int(args, &rate))
> + return -EINVAL;
>
> if (rate) {
> atomic_set(&ffi->inject_ops, 0);
> @@ -157,6 +161,16 @@ static void erofs_build_fault_attr(struct erofs_sb_info
> *sbi,
> } else {
> memset(ffi, 0, sizeof(struct erofs_fault_info));
> }
> +
> + set_opt(sbi, FAULT_INJECTION);
> + return 0;
> +}
> +#else
> +static int erofs_build_fault_attr(struct erofs_sb_info *sbi,
> + substring_t *args)
> +{
> + infoln("fault_injection options not supported");
> + return 0;
> }
> #endif
>
> @@ -193,7 +207,7 @@ static int parse_options(struct super_block *sb, char
> *options)
> {
> substring_t args[MAX_OPT_ARGS];
> char *p;
> - int arg = 0;
> + int err;
>
> if (!options)
> return 0;
> @@ -238,18 +252,12 @@ static int parse_options(struct super_block *sb, char
> *options)
> infoln("noacl options not supported");
> break;
> #endif
> -#ifdef CONFIG_EROFS_FAULT_INJECTION
> case Opt_fault_injection:
> - if (args->from && match_int(args, &arg))
> - return -EINVAL;
> - erofs_build_fault_attr(EROFS_SB(sb), arg);
> - set_opt(EROFS_SB(sb), FAULT_INJECTION);
> + err = erofs_build_fault_attr(EROFS_SB(sb), args);
> + if (err)
> + return err;
> break;
> -#else
> - case Opt_fault_injection:
> - infoln("fault_injection options not supported");
> - break;
> -#endif
> +
> default:
> errln("Unrecognized mount option \"%s\" "
> "or missing value", p);
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 3/4] staging: erofs: code cleanup for erofs_show_options()
On 2018/9/18 23:10, Chengguang Xu wrote: > Add new helper erofs_get_fault_rate() to get fault rate instead of > directly getting it from sbi, so we can remove the macro check > surrounding it. > > Signed-off-by: Chengguang Xu > Reviewed-by: Chao Yu Reviewed-by: Gao Xiang Thanks, Gao Xiang ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 4/4] staging: erofs: option validation in remount
Hi Chengguang,
On 2018/9/18 23:10, Chengguang Xu wrote:
> Add option validation in remount. After this patch, remount
> can change recognized options, and for unknown options remount
> will fail and report error.
>
> Signed-off-by: Chengguang Xu
> Reviewed-by: Chao Yu
> ---
> drivers/staging/erofs/super.c | 38 ---
> 1 file changed, 31 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/staging/erofs/super.c b/drivers/staging/erofs/super.c
> index a091b93190e1..30b6b44dc6c4 100644
> --- a/drivers/staging/erofs/super.c
> +++ b/drivers/staging/erofs/super.c
> @@ -145,14 +145,10 @@ char *erofs_fault_name[FAULT_MAX] = {
> [FAULT_KMALLOC] = "kmalloc",
> };
>
> -static int erofs_build_fault_attr(struct erofs_sb_info *sbi,
> - substring_t *args)
> +static void __erofs_build_fault_attr(struct erofs_sb_info *sbi,
> + unsigned int rate)
> {
> struct erofs_fault_info *ffi = &sbi->fault_info;
> - int rate = 0;
> -
> - if (args->from && match_int(args, &rate))
> - return -EINVAL;
>
> if (rate) {
> atomic_set(&ffi->inject_ops, 0);
> @@ -163,14 +159,29 @@ static int erofs_build_fault_attr(struct erofs_sb_info
> *sbi,
> }
>
> set_opt(sbi, FAULT_INJECTION);
> - return 0;
> }
>
> static unsigned int erofs_get_fault_rate(struct erofs_sb_info *sbi)
> {
> return sbi->fault_info.inject_rate;
> }
> +
> +static int erofs_build_fault_attr(struct erofs_sb_info *sbi, substring_t
> *args)
> +{
> + int rate = 0;
> +
> + if (args->from && match_int(args, &rate))
> + return -EINVAL;
> +
> + __erofs_build_fault_attr(sbi, rate);
> + return 0;
> +}
> #else
> +static void __erofs_build_fault_attr(struct erofs_sb_info *sbi,
> + unsigned int rate)
> +{
> +}
> +
> static int erofs_build_fault_attr(struct erofs_sb_info *sbi,
> substring_t *args)
> {
> @@ -644,10 +655,23 @@ static int erofs_show_options(struct seq_file *seq,
> struct dentry *root)
>
> static int erofs_remount(struct super_block *sb, int *flags, char *data)
> {
> + struct erofs_sb_info *sbi = EROFS_SB(sb);
> + unsigned int org_mnt_opt = sbi->mount_opt;
> + unsigned int org_inject_rate = erofs_get_fault_rate(sbi);
> + int err;
> +
> BUG_ON(!sb_rdonly(sb));
> + err = parse_options(sb, data);
> + if (err)
> + goto out;
>
> *flags |= MS_RDONLY;
I cannot apply this patch since the above line has been changed in
5f0abea6ab6d("staging: erofs: rename superblock flags (MS_xyz -> SB_xyz)").
And two more checkpatch CHECKs as [PATCH v3 2/4]
CHECK: Alignment should match open parenthesis
#141: FILE: drivers/staging/erofs/super.c:149:
+static void __erofs_build_fault_attr(struct erofs_sb_info *sbi,
+ unsigned int rate)
CHECK: Alignment should match open parenthesis
#175: FILE: drivers/staging/erofs/super.c:181:
+static void __erofs_build_fault_attr(struct erofs_sb_info *sbi,
+ unsigned int rate)
And I think I need to test the basic erofs remount function tomorrow
before merging... it is too late and I have to go to sleep now...
Thanks,
Gao Xiang
> return 0;
> +out:
> + __erofs_build_fault_attr(sbi, org_inject_rate);
> + sbi->mount_opt = org_mnt_opt;
> +
> + return err;
> }
>
> const struct super_operations erofs_sops = {
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 2/6] staging: erofs: fold in `__update_workgrp_llen'
There is the only one user to use `__update_workgrp_llen'.
Fold it in `z_erofs_vle_work_iter_begin' and cleanup related code.
Signed-off-by: Gao Xiang
---
drivers/staging/erofs/unzip_vle.c | 20 +++-
1 file changed, 7 insertions(+), 13 deletions(-)
diff --git a/drivers/staging/erofs/unzip_vle.c
b/drivers/staging/erofs/unzip_vle.c
index 2b9b313..e820490 100644
--- a/drivers/staging/erofs/unzip_vle.c
+++ b/drivers/staging/erofs/unzip_vle.c
@@ -422,18 +422,6 @@ struct z_erofs_vle_work_finder {
return work;
}
-static inline void __update_workgrp_llen(struct z_erofs_vle_workgroup *grp,
-unsigned int llen)
-{
- while (1) {
- unsigned int orig_llen = grp->llen;
-
- if (orig_llen >= llen || orig_llen ==
- cmpxchg(&grp->llen, orig_llen, llen))
- break;
- }
-}
-
#define builder_is_followed(builder) \
((builder)->role >= Z_EROFS_VLE_WORK_PRIMARY_FOLLOWED)
@@ -466,7 +454,13 @@ static int z_erofs_vle_work_iter_begin(struct
z_erofs_vle_work_builder *builder,
repeat:
work = z_erofs_vle_work_lookup(&finder);
if (work != NULL) {
- __update_workgrp_llen(grp, map->m_llen);
+ unsigned int orig_llen;
+
+ /* increase workgroup `llen' if needed */
+ while ((orig_llen = READ_ONCE(grp->llen)) < map->m_llen &&
+ orig_llen != cmpxchg_relaxed(&grp->llen,
+ orig_llen, map->m_llen))
+ cpu_relax();
goto got_it;
}
--
1.9.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 1/6] staging: erofs: remove redundant CONFIG_EROFS_FS_XATTRs
some CONFIG_EROFS_FS_XATTR conditions were added because of
the historial Linux kernel compatibility, which are unneeded now.
Signed-off-by: Gao Xiang
---
Hi all,
These are cleanup patches in Chao's erofs-dev test tree for a while.
Many of them are trivial.
In order for all preview patches to keep up with the staging tree,
I resend them now.
Thanks,
Gao Xiang
drivers/staging/erofs/inode.c| 6 --
drivers/staging/erofs/internal.h | 6 ++
2 files changed, 2 insertions(+), 10 deletions(-)
diff --git a/drivers/staging/erofs/inode.c b/drivers/staging/erofs/inode.c
index c46a8d4..da8693a 100644
--- a/drivers/staging/erofs/inode.c
+++ b/drivers/staging/erofs/inode.c
@@ -260,22 +260,16 @@ struct inode *erofs_iget(struct super_block *sb,
const struct inode_operations erofs_generic_xattr_iops = {
.listxattr = erofs_listxattr,
};
-#endif
-#ifdef CONFIG_EROFS_FS_XATTR
const struct inode_operations erofs_symlink_xattr_iops = {
.get_link = page_get_link,
.listxattr = erofs_listxattr,
};
-#endif
const struct inode_operations erofs_special_inode_operations = {
-#ifdef CONFIG_EROFS_FS_XATTR
.listxattr = erofs_listxattr,
-#endif
};
-#ifdef CONFIG_EROFS_FS_XATTR
const struct inode_operations erofs_fast_symlink_xattr_iops = {
.get_link = simple_get_link,
.listxattr = erofs_listxattr,
diff --git a/drivers/staging/erofs/internal.h b/drivers/staging/erofs/internal.h
index 0011b9d..cfcc6db 100644
--- a/drivers/staging/erofs/internal.h
+++ b/drivers/staging/erofs/internal.h
@@ -507,13 +507,11 @@ extern struct inode *erofs_iget(struct super_block *sb,
int erofs_namei(struct inode *dir, struct qstr *name,
erofs_nid_t *nid, unsigned *d_type);
-/* xattr.c */
#ifdef CONFIG_EROFS_FS_XATTR
+/* xattr.c */
extern const struct xattr_handler *erofs_xattr_handlers[];
-#endif
-/* symlink */
-#ifdef CONFIG_EROFS_FS_XATTR
+/* symlink and special inode */
extern const struct inode_operations erofs_symlink_xattr_iops;
extern const struct inode_operations erofs_fast_symlink_xattr_iops;
extern const struct inode_operations erofs_special_inode_operations;
--
1.9.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 3/6] staging: erofs: drop multiref support temporarily
Multiref support means that a compressed page could have
more than one reference, which is designed for on-disk data
deduplication. However, mkfs doesn't support this mode
at this moment, and the kernel implementation is also broken.
Let's drop multiref support. If it is fully implemented
in the future, it can be reverted later.
Signed-off-by: Gao Xiang
---
drivers/staging/erofs/unzip_vle.c | 36 +---
drivers/staging/erofs/unzip_vle.h | 12 +---
2 files changed, 6 insertions(+), 42 deletions(-)
diff --git a/drivers/staging/erofs/unzip_vle.c
b/drivers/staging/erofs/unzip_vle.c
index e820490..9fe18cd3 100644
--- a/drivers/staging/erofs/unzip_vle.c
+++ b/drivers/staging/erofs/unzip_vle.c
@@ -301,12 +301,9 @@ struct z_erofs_vle_work_finder {
grp = container_of(egrp, struct z_erofs_vle_workgroup, obj);
*f->grp_ret = grp;
-#ifndef CONFIG_EROFS_FS_ZIP_MULTIREF
work = z_erofs_vle_grab_work(grp, f->pageofs);
+ /* if multiref is disabled, `primary' is always true */
primary = true;
-#else
- BUG();
-#endif
DBG_BUGON(work->pageofs != f->pageofs);
@@ -368,12 +365,9 @@ struct z_erofs_vle_work_finder {
struct z_erofs_vle_workgroup *grp = *f->grp_ret;
struct z_erofs_vle_work *work;
-#ifndef CONFIG_EROFS_FS_ZIP_MULTIREF
+ /* if multiref is disabled, grp should never be nullptr */
BUG_ON(grp != NULL);
-#else
- if (grp != NULL)
- goto skip;
-#endif
+
/* no available workgroup, let's allocate one */
grp = kmem_cache_zalloc(z_erofs_workgroup_cachep, GFP_NOFS);
if (unlikely(grp == NULL))
@@ -396,13 +390,7 @@ struct z_erofs_vle_work_finder {
*f->hosted = true;
gnew = true;
-#ifdef CONFIG_EROFS_FS_ZIP_MULTIREF
-skip:
- /* currently unimplemented */
- BUG();
-#else
work = z_erofs_vle_grab_primary_work(grp);
-#endif
work->pageofs = f->pageofs;
mutex_init(&work->lock);
@@ -797,9 +785,7 @@ static int z_erofs_vle_unzip(struct super_block *sb,
struct z_erofs_pagevec_ctor ctor;
unsigned int nr_pages;
-#ifndef CONFIG_EROFS_FS_ZIP_MULTIREF
unsigned int sparsemem_pages = 0;
-#endif
struct page *pages_onstack[Z_EROFS_VLE_VMAP_ONSTACK_PAGES];
struct page **pages, **compressed_pages, *page;
unsigned int i, llen;
@@ -811,11 +797,7 @@ static int z_erofs_vle_unzip(struct super_block *sb,
int err;
might_sleep();
-#ifndef CONFIG_EROFS_FS_ZIP_MULTIREF
work = z_erofs_vle_grab_primary_work(grp);
-#else
- BUG();
-#endif
BUG_ON(!READ_ONCE(work->nr_pages));
mutex_lock(&work->lock);
@@ -866,13 +848,11 @@ static int z_erofs_vle_unzip(struct super_block *sb,
pagenr = z_erofs_onlinepage_index(page);
BUG_ON(pagenr >= nr_pages);
-
-#ifndef CONFIG_EROFS_FS_ZIP_MULTIREF
BUG_ON(pages[pagenr] != NULL);
- ++sparsemem_pages;
-#endif
+
pages[pagenr] = page;
}
+ sparsemem_pages = i;
z_erofs_pagevec_ctor_exit(&ctor, true);
@@ -902,10 +882,8 @@ static int z_erofs_vle_unzip(struct super_block *sb,
pagenr = z_erofs_onlinepage_index(page);
BUG_ON(pagenr >= nr_pages);
-#ifndef CONFIG_EROFS_FS_ZIP_MULTIREF
BUG_ON(pages[pagenr] != NULL);
++sparsemem_pages;
-#endif
pages[pagenr] = page;
overlapped = true;
@@ -931,12 +909,10 @@ static int z_erofs_vle_unzip(struct super_block *sb,
if (err != -ENOTSUPP)
goto out_percpu;
-#ifndef CONFIG_EROFS_FS_ZIP_MULTIREF
if (sparsemem_pages >= nr_pages) {
BUG_ON(sparsemem_pages > nr_pages);
goto skip_allocpage;
}
-#endif
for (i = 0; i < nr_pages; ++i) {
if (pages[i] != NULL)
@@ -945,9 +921,7 @@ static int z_erofs_vle_unzip(struct super_block *sb,
pages[i] = __stagingpage_alloc(page_pool, GFP_NOFS);
}
-#ifndef CONFIG_EROFS_FS_ZIP_MULTIREF
skip_allocpage:
-#endif
vout = erofs_vmap(pages, nr_pages);
err = z_erofs_vle_unzip_vmap(compressed_pages,
diff --git a/drivers/staging/erofs/unzip_vle.h
b/drivers/staging/erofs/unzip_vle.h
index 3939985..3316bc3 100644
--- a/drivers/staging/erofs/unzip_vle.h
+++ b/drivers/staging/erofs/unzip_vle.h
@@ -47,13 +47,6 @@ static inline bool z_erofs_gather_if_stagingpage(struct
list_head *page_pool,
#define Z_EROFS_VLE_INLINE_PAGEVECS 3
struct z_erofs_vle_work {
- /* struct z_erofs_vle_work *left, *right; */
-
-#ifdef CONFIG_EROFS_FS_ZIP_MULTIREF
- struct list_head list;
-
- atomic_t refcount;
-#endif
struct mutex lock;
/* I: decompression offset in page */
@@ -107,10 +100,8 @@ static
[PATCH 4/6] staging: erofs: cleanup `z_erofs_vle_normalaccess_readpages'
This patch introduces `__should_decompress_synchronously' to
cleanup `z_erofs_vle_normalaccess_readpages'.
Signed-off-by: Gao Xiang
---
drivers/staging/erofs/internal.h | 11 +++
drivers/staging/erofs/super.c | 5 +
drivers/staging/erofs/unzip_vle.c | 20 ++--
3 files changed, 22 insertions(+), 14 deletions(-)
diff --git a/drivers/staging/erofs/internal.h b/drivers/staging/erofs/internal.h
index cfcc6db..c84eb97 100644
--- a/drivers/staging/erofs/internal.h
+++ b/drivers/staging/erofs/internal.h
@@ -95,6 +95,9 @@ struct erofs_sb_info {
/* the dedicated workstation for compression */
struct radix_tree_root workstn_tree;
+ /* threshold for decompression synchronously */
+ unsigned int max_sync_decompress_pages;
+
#ifdef EROFS_FS_HAS_MANAGED_CACHE
struct inode *managed_cache;
#endif
@@ -273,6 +276,14 @@ extern int erofs_try_to_free_cached_page(struct
address_space *mapping,
struct page *page);
#endif
+#define DEFAULT_MAX_SYNC_DECOMPRESS_PAGES 4
+
+static inline bool __should_decompress_synchronously(struct erofs_sb_info *sbi,
+unsigned int nr)
+{
+ return nr <= sbi->max_sync_decompress_pages;
+}
+
#endif
/* we strictly follow PAGE_SIZE and no buffer head yet */
diff --git a/drivers/staging/erofs/super.c b/drivers/staging/erofs/super.c
index 802202c..5b87f59 100644
--- a/drivers/staging/erofs/super.c
+++ b/drivers/staging/erofs/super.c
@@ -162,6 +162,11 @@ static void erofs_build_fault_attr(struct erofs_sb_info
*sbi,
static void default_options(struct erofs_sb_info *sbi)
{
+ /* set up some FS parameters */
+#ifdef CONFIG_EROFS_FS_ZIP
+ sbi->max_sync_decompress_pages = DEFAULT_MAX_SYNC_DECOMPRESS_PAGES;
+#endif
+
#ifdef CONFIG_EROFS_FS_XATTR
set_opt(sbi, XATTR_USER);
#endif
diff --git a/drivers/staging/erofs/unzip_vle.c
b/drivers/staging/erofs/unzip_vle.c
index 9fe18cd3..337a82d 100644
--- a/drivers/staging/erofs/unzip_vle.c
+++ b/drivers/staging/erofs/unzip_vle.c
@@ -1308,12 +1308,14 @@ static int z_erofs_vle_normalaccess_readpage(struct
file *file,
return 0;
}
-static inline int __z_erofs_vle_normalaccess_readpages(
- struct file *filp,
- struct address_space *mapping,
- struct list_head *pages, unsigned int nr_pages, bool sync)
+static int z_erofs_vle_normalaccess_readpages(struct file *filp,
+ struct address_space *mapping,
+ struct list_head *pages,
+ unsigned int nr_pages)
{
struct inode *const inode = mapping->host;
+ struct erofs_sb_info *const sbi = EROFS_I_SB(inode);
+ const bool sync = __should_decompress_synchronously(sbi, nr_pages);
struct z_erofs_vle_frontend f = VLE_FRONTEND_INIT(inode);
gfp_t gfp = mapping_gfp_constraint(mapping, GFP_KERNEL);
@@ -1372,16 +1374,6 @@ static inline int __z_erofs_vle_normalaccess_readpages(
return 0;
}
-static int z_erofs_vle_normalaccess_readpages(
- struct file *filp,
- struct address_space *mapping,
- struct list_head *pages, unsigned int nr_pages)
-{
- return __z_erofs_vle_normalaccess_readpages(filp,
- mapping, pages, nr_pages,
- nr_pages < 4 /* sync */);
-}
-
const struct address_space_operations z_erofs_vle_normalaccess_aops = {
.readpage = z_erofs_vle_normalaccess_readpage,
.readpages = z_erofs_vle_normalaccess_readpages,
--
1.9.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 5/6] staging: erofs: add some comments for xattr subsystem
As Dan Carpenter pointed out, it is better to document what
return values of these callbacks in `struct xattr_iter_handlers'
mean and why it->ofs is increased regardless of success or
failure in `xattr_foreach'.
Suggested-by: Dan Carpenter
Signed-off-by: Gao Xiang
---
drivers/staging/erofs/xattr.c | 13 -
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/erofs/xattr.c b/drivers/staging/erofs/xattr.c
index 4942ca1..7b1367e 100644
--- a/drivers/staging/erofs/xattr.c
+++ b/drivers/staging/erofs/xattr.c
@@ -109,6 +109,13 @@ static int init_inode_xattrs(struct inode *inode)
return 0;
}
+/*
+ * the general idea for these return values is
+ * if0 is returned, go on processing the current xattr;
+ * 1 (> 0) is returned, skip this round to process the next xattr;
+ *-err (< 0) is returned, an error (maybe ENOXATTR) occurred
+ *and need to be handled
+ */
struct xattr_iter_handlers {
int (*entry)(struct xattr_iter *, struct erofs_xattr_entry *);
int (*name)(struct xattr_iter *, unsigned int, char *, unsigned int);
@@ -164,6 +171,10 @@ static int inline_xattr_iter_begin(struct xattr_iter *it,
return vi->xattr_isize - xattr_header_sz;
}
+/*
+ * Regardless of success or failure, `xattr_foreach' will end up with
+ * `ofs' pointing to the next xattr item rather than an arbitrary position.
+ */
static int xattr_foreach(struct xattr_iter *it,
const struct xattr_iter_handlers *op, unsigned int *tlimit)
{
@@ -255,7 +266,7 @@ static int xattr_foreach(struct xattr_iter *it,
}
out:
- /* we assume that ofs is aligned with 4 bytes */
+ /* xattrs should be 4-byte aligned (on-disk constraint) */
it->ofs = EROFS_XATTR_ALIGN(it->ofs);
return err;
}
--
1.9.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 6/6] staging: erofs: simplify return value of `xattr_foreach'
As Dan Carpenter pointed out, there is no need to propagate positive
return values back to its callers.
Suggested-by: Dan Carpenter
Signed-off-by: Gao Xiang
---
drivers/staging/erofs/xattr.c | 24 +---
1 file changed, 9 insertions(+), 15 deletions(-)
diff --git a/drivers/staging/erofs/xattr.c b/drivers/staging/erofs/xattr.c
index 7b1367e..80dca6a 100644
--- a/drivers/staging/erofs/xattr.c
+++ b/drivers/staging/erofs/xattr.c
@@ -268,7 +268,7 @@ static int xattr_foreach(struct xattr_iter *it,
out:
/* xattrs should be 4-byte aligned (on-disk constraint) */
it->ofs = EROFS_XATTR_ALIGN(it->ofs);
- return err;
+ return err < 0 ? err : 0;
}
struct getxattr_iter {
@@ -333,15 +333,12 @@ static int inline_getxattr(struct inode *inode, struct
getxattr_iter *it)
remaining = ret;
while (remaining) {
ret = xattr_foreach(&it->it, &find_xattr_handlers, &remaining);
- if (ret >= 0)
- break;
-
- if (ret != -ENOATTR)/* -ENOMEM, -EIO, etc. */
+ if (ret != -ENOATTR)
break;
}
xattr_iter_end_final(&it->it);
- return ret < 0 ? ret : it->buffer_size;
+ return ret ? ret : it->buffer_size;
}
static int shared_getxattr(struct inode *inode, struct getxattr_iter *it)
@@ -371,16 +368,13 @@ static int shared_getxattr(struct inode *inode, struct
getxattr_iter *it)
}
ret = xattr_foreach(&it->it, &find_xattr_handlers, NULL);
- if (ret >= 0)
- break;
-
- if (ret != -ENOATTR)/* -ENOMEM, -EIO, etc. */
+ if (ret != -ENOATTR)
break;
}
if (vi->xattr_shared_count)
xattr_iter_end_final(&it->it);
- return ret < 0 ? ret : it->buffer_size;
+ return ret ? ret : it->buffer_size;
}
static bool erofs_xattr_user_list(struct dentry *dentry)
@@ -567,11 +561,11 @@ static int inline_listxattr(struct listxattr_iter *it)
remaining = ret;
while (remaining) {
ret = xattr_foreach(&it->it, &list_xattr_handlers, &remaining);
- if (ret < 0)
+ if (ret)
break;
}
xattr_iter_end_final(&it->it);
- return ret < 0 ? ret : it->buffer_ofs;
+ return ret ? ret : it->buffer_ofs;
}
static int shared_listxattr(struct listxattr_iter *it)
@@ -601,13 +595,13 @@ static int shared_listxattr(struct listxattr_iter *it)
}
ret = xattr_foreach(&it->it, &list_xattr_handlers, NULL);
- if (ret < 0)
+ if (ret)
break;
}
if (vi->xattr_shared_count)
xattr_iter_end_final(&it->it);
- return ret < 0 ? ret : it->buffer_ofs;
+ return ret ? ret : it->buffer_ofs;
}
ssize_t erofs_listxattr(struct dentry *dentry,
--
1.9.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 4/4] staging: erofs: option validation in remount
Hi Chengguang,
On 2018/9/19 0:58, Gao Xiang wrote:
> Hi Chengguang,
>
> On 2018/9/18 23:10, Chengguang Xu wrote:
>> Add option validation in remount. After this patch, remount
>> can change recognized options, and for unknown options remount
>> will fail and report error.
>>
>> Signed-off-by: Chengguang Xu
>> Reviewed-by: Chao Yu
>> ---
>> drivers/staging/erofs/super.c | 38 ---
>> 1 file changed, 31 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/staging/erofs/super.c b/drivers/staging/erofs/super.c
>> index a091b93190e1..30b6b44dc6c4 100644
>> --- a/drivers/staging/erofs/super.c
>> +++ b/drivers/staging/erofs/super.c
>> @@ -145,14 +145,10 @@ char *erofs_fault_name[FAULT_MAX] = {
>> [FAULT_KMALLOC] = "kmalloc",
>> };
>>
>> -static int erofs_build_fault_attr(struct erofs_sb_info *sbi,
>> -substring_t *args)
>> +static void __erofs_build_fault_attr(struct erofs_sb_info *sbi,
>> +unsigned int rate)
>> {
>> struct erofs_fault_info *ffi = &sbi->fault_info;
>> -int rate = 0;
>> -
>> -if (args->from && match_int(args, &rate))
>> -return -EINVAL;
>>
>> if (rate) {
>> atomic_set(&ffi->inject_ops, 0);
>> @@ -163,14 +159,29 @@ static int erofs_build_fault_attr(struct erofs_sb_info
>> *sbi,
>> }
>>
>> set_opt(sbi, FAULT_INJECTION);
>> -return 0;
>> }
>>
>> static unsigned int erofs_get_fault_rate(struct erofs_sb_info *sbi)
>> {
>> return sbi->fault_info.inject_rate;
>> }
>> +
>> +static int erofs_build_fault_attr(struct erofs_sb_info *sbi, substring_t
>> *args)
>> +{
>> +int rate = 0;
>> +
>> +if (args->from && match_int(args, &rate))
>> +return -EINVAL;
>> +
>> +__erofs_build_fault_attr(sbi, rate);
>> +return 0;
>> +}
>> #else
>> +static void __erofs_build_fault_attr(struct erofs_sb_info *sbi,
>> +unsigned int rate)
>> +{
>> +}
>> +
>> static int erofs_build_fault_attr(struct erofs_sb_info *sbi,
>> substring_t *args)
>> {
>> @@ -644,10 +655,23 @@ static int erofs_show_options(struct seq_file *seq,
>> struct dentry *root)
>>
>> static int erofs_remount(struct super_block *sb, int *flags, char *data)
>> {
>> +struct erofs_sb_info *sbi = EROFS_SB(sb);
>> +unsigned int org_mnt_opt = sbi->mount_opt;
>> +unsigned int org_inject_rate = erofs_get_fault_rate(sbi);
>> +int err;
>> +
>> BUG_ON(!sb_rdonly(sb));
>> +err = parse_options(sb, data);
>> +if (err)
>> +goto out;
>>
>> *flags |= MS_RDONLY;
> I cannot apply this patch since the above line has been changed in
> 5f0abea6ab6d("staging: erofs: rename superblock flags (MS_xyz -> SB_xyz)").
>
>
> And two more checkpatch CHECKs as [PATCH v3 2/4]
>
> CHECK: Alignment should match open parenthesis
> #141: FILE: drivers/staging/erofs/super.c:149:
> +static void __erofs_build_fault_attr(struct erofs_sb_info *sbi,
> + unsigned int rate)
>
> CHECK: Alignment should match open parenthesis
> #175: FILE: drivers/staging/erofs/super.c:181:
> +static void __erofs_build_fault_attr(struct erofs_sb_info *sbi,
> + unsigned int rate)
>
> And I think I need to test the basic erofs remount function tomorrow
> before merging... it is too late and I have to go to sleep now.
I tested just now, and seems good. :)
Reviewed-by: Gao Xiang
and look forward to your final patchset.
Thanks,
Gao Xiang
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 0/3] staging: erofs: option validation for remount and some code cleanups
Hi Chengguang, On 2018/9/19 22:53, Chengguang Xu wrote: > Hi Greg, Xiang > > I rebased code on latest erofs-master branch and that branch > has already merged the first patch in my previous patchset, > so this time I only post rest 3 patches. Great, at the most time Chao's erofs-master is the same as Greg's staging tree (currently it is), but I personally think all patches could be better directly based on the final staging tree if these patches has no real/effective dependency with some submitted patches but haven't been applied by Greg (p.s. it should be avoided as much as possible because community guys could find something important like the yesterday patches). Once again, the detail information of branches is described in https://lists.ozlabs.org/listinfo/linux-erofs/ I have nothing more to say, good luck :) Thanks, Gao Xiang > > Thanks, > > -- > > This patch set mainly does option validation for remount and at > the same time does related code cleanups. Currently when we call > fault injection related code we have to surround it with macro > CONFIG_EROFS_FAULT_INJECTION in every calling place, after this > patch set we don't have to do that, so the code looks clean and > more understandable. > > v3->v4: > - Rebase code on latest erofs-master branch in Chao's linux tree. > - Fix checkpatch complains. > > v2->v3: > - Fold related patches to one patch. > - Fix building issue. > > v1->v2: > Address Chao's comments: > - Allow to set fault_injection=0. > - Keep flag bit when setting fault_injection=0. > - Show injection info in original place. > - Rebase code on latest erofs branch in Chao's linux tree. > - Fix building issue. > > Chengguang Xu (3): > staging: erofs: code cleanup for option parsing of fault_injection > staging: erofs: code cleanup for erofs_show_options() > staging: erofs: option validation in remount > > drivers/staging/erofs/super.c | 73 +++ > 1 file changed, 57 insertions(+), 16 deletions(-) > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 4/6] staging: erofs: cleanup `z_erofs_vle_normalaccess_readpages'
Hi Chao,
On 2018/9/19 23:26, Chao Yu wrote:
> Hi Xiang,
>
> On 2018/9/19 13:49, Gao Xiang wrote:
>> This patch introduces `__should_decompress_synchronously' to
>> cleanup `z_erofs_vle_normalaccess_readpages'.
>>
>> Signed-off-by: Gao Xiang
>> ---
>> drivers/staging/erofs/internal.h | 11 +++
>> drivers/staging/erofs/super.c | 5 +
>> drivers/staging/erofs/unzip_vle.c | 20 ++--
>> 3 files changed, 22 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/staging/erofs/internal.h
>> b/drivers/staging/erofs/internal.h
>> index cfcc6db..c84eb97 100644
>> --- a/drivers/staging/erofs/internal.h
>> +++ b/drivers/staging/erofs/internal.h
>> @@ -95,6 +95,9 @@ struct erofs_sb_info {
>> /* the dedicated workstation for compression */
>> struct radix_tree_root workstn_tree;
>>
>> +/* threshold for decompression synchronously */
>> +unsigned int max_sync_decompress_pages;
>> +
>> #ifdef EROFS_FS_HAS_MANAGED_CACHE
>> struct inode *managed_cache;
>> #endif
>> @@ -273,6 +276,14 @@ extern int erofs_try_to_free_cached_page(struct
>> address_space *mapping,
>> struct page *page);
>> #endif
>>
>> +#define DEFAULT_MAX_SYNC_DECOMPRESS_PAGES 4
>> +
>> +static inline bool __should_decompress_synchronously(struct erofs_sb_info
>> *sbi,
>> + unsigned int nr)
>> +{
>> +return nr <= sbi->max_sync_decompress_pages;
> - nr_pages < 4 /* sync */);
>
> There is a little bit changed except cleanup, IIUC, would it be any difference
> of performance around boundary of four pages?
No.. Once synchronous decompression is applied for 1,2,3 pages for no special
reasons.
But I think it could be better to adjust it to the power of two --- 1,2,3,4 is
preferred.
Since I have no idea to measure which is better or what value is best for all
platform or use cases...
Therefore I tune it in this patch since I don't like the number
DEFAULT_MAX_SYNC_DECOMPRESS_PAGES == 3 ...
Thanks,
Gao Xiang
>
> Thanks,
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 4/6] staging: erofs: cleanup `z_erofs_vle_normalaccess_readpages'
Hi Chao,
On 2018/9/19 23:32, Gao Xiang via Linux-erofs wrote:
> Hi Chao,
>
> On 2018/9/19 23:26, Chao Yu wrote:
>> Hi Xiang,
>>
>> On 2018/9/19 13:49, Gao Xiang wrote:
>>> This patch introduces `__should_decompress_synchronously' to
>>> cleanup `z_erofs_vle_normalaccess_readpages'.
>>>
>>> Signed-off-by: Gao Xiang
>>> ---
>>> drivers/staging/erofs/internal.h | 11 +++
>>> drivers/staging/erofs/super.c | 5 +
>>> drivers/staging/erofs/unzip_vle.c | 20 ++--
>>> 3 files changed, 22 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/staging/erofs/internal.h
>>> b/drivers/staging/erofs/internal.h
>>> index cfcc6db..c84eb97 100644
>>> --- a/drivers/staging/erofs/internal.h
>>> +++ b/drivers/staging/erofs/internal.h
>>> @@ -95,6 +95,9 @@ struct erofs_sb_info {
>>> /* the dedicated workstation for compression */
>>> struct radix_tree_root workstn_tree;
>>>
>>> + /* threshold for decompression synchronously */
>>> + unsigned int max_sync_decompress_pages;
>>> +
>>> #ifdef EROFS_FS_HAS_MANAGED_CACHE
>>> struct inode *managed_cache;
>>> #endif
>>> @@ -273,6 +276,14 @@ extern int erofs_try_to_free_cached_page(struct
>>> address_space *mapping,
>>> struct page *page);
>>> #endif
>>>
>>> +#define DEFAULT_MAX_SYNC_DECOMPRESS_PAGES 4
>>> +
>>> +static inline bool __should_decompress_synchronously(struct erofs_sb_info
>>> *sbi,
>>> +unsigned int nr)
>>> +{
>>> + return nr <= sbi->max_sync_decompress_pages;
>> -nr_pages < 4 /* sync */);
>>
>> There is a little bit changed except cleanup, IIUC, would it be any
>> difference
>> of performance around boundary of four pages?
>
> No.. Once synchronous decompression is applied for 1,2,3 pages for no special
> reasons.
> But I think it could be better to adjust it to the power of two --- 1,2,3,4
> is preferred.
> Since I have no idea to measure which is better or what value is best for all
> platform or use cases...
>
> Therefore I tune it in this patch since I don't like the number
> DEFAULT_MAX_SYNC_DECOMPRESS_PAGES == 3 ...
p.s. If you think that is not the cleanup meaning, I will resend this patch with
the original 3. Both are ok for me. it is a minor tuning and I don't have a way
to measure which is better and the best boundary.
Thanks,
Gao Xiang
>
> Thanks,
> Gao Xiang
>
>>
>> Thanks,
>>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 4/6] staging: erofs: cleanup `z_erofs_vle_normalaccess_readpages'
Hi Chao,
On 2018/9/19 23:45, Chao Yu wrote:
> Hi Xiang,
>
> On 2018/9/19 23:32, Gao Xiang wrote:
>> Hi Chao,
>>
>> On 2018/9/19 23:26, Chao Yu wrote:
>>> Hi Xiang,
>>>
>>> On 2018/9/19 13:49, Gao Xiang wrote:
>>>> This patch introduces `__should_decompress_synchronously' to
>>>> cleanup `z_erofs_vle_normalaccess_readpages'.
>>>>
>>>> Signed-off-by: Gao Xiang
>>>> ---
>>>> drivers/staging/erofs/internal.h | 11 +++
>>>> drivers/staging/erofs/super.c | 5 +
>>>> drivers/staging/erofs/unzip_vle.c | 20 ++--
>>>> 3 files changed, 22 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/drivers/staging/erofs/internal.h
>>>> b/drivers/staging/erofs/internal.h
>>>> index cfcc6db..c84eb97 100644
>>>> --- a/drivers/staging/erofs/internal.h
>>>> +++ b/drivers/staging/erofs/internal.h
>>>> @@ -95,6 +95,9 @@ struct erofs_sb_info {
>>>>/* the dedicated workstation for compression */
>>>>struct radix_tree_root workstn_tree;
>>>>
>>>> + /* threshold for decompression synchronously */
>>>> + unsigned int max_sync_decompress_pages;
>>>> +
>>>> #ifdef EROFS_FS_HAS_MANAGED_CACHE
>>>>struct inode *managed_cache;
>>>> #endif
>>>> @@ -273,6 +276,14 @@ extern int erofs_try_to_free_cached_page(struct
>>>> address_space *mapping,
>>>>struct page *page);
>>>> #endif
>>>>
>>>> +#define DEFAULT_MAX_SYNC_DECOMPRESS_PAGES 4
>>>> +
>>>> +static inline bool __should_decompress_synchronously(struct erofs_sb_info
>>>> *sbi,
>>>> + unsigned int nr)
>>>> +{
>>>> + return nr <= sbi->max_sync_decompress_pages;
>>> - nr_pages < 4 /* sync */);
>>>
>>> There is a little bit changed except cleanup, IIUC, would it be any
>>> difference
>>> of performance around boundary of four pages?
>>
>> No.. Once synchronous decompression is applied for 1,2,3 pages for no
>> special reasons.
>> But I think it could be better to adjust it to the power of two --- 1,2,3,4
>> is preferred.
>> Since I have no idea to measure which is better or what value is best for
>> all platform or use cases...
>>
>> Therefore I tune it in this patch since I don't like the number
>> DEFAULT_MAX_SYNC_DECOMPRESS_PAGES == 3 ...
>
> Yeah, how about adding one more patch to tune this magic number first? as that
> change would not be part of cleanup thing. I guess later maybe we could
> export a
> sysfs node to provider tuning ability on that threshold.
OK, I think we can leave the original "3" for now. I will fix that value and
resend this patch soon.
The sysfs interface could be necessary later since it seems some arguments
involved in decompression could impact the decompression performance.
Thanks,
Gao Xiang
>
> Thanks,
>
>>
>> Thanks,
>> Gao Xiang
>>
>>>
>>> Thanks,
>>>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 4/6] staging: erofs: cleanup `z_erofs_vle_normalaccess_readpages'
From: Gao Xiang
This patch introduces `__should_decompress_synchronously' to
cleanup `z_erofs_vle_normalaccess_readpages'.
Signed-off-by: Gao Xiang
---
change log v2:
- Leave the original threshold "3" for DEFAULT_MAX_SYNC_DECOMPRESS_PAGES
and just do the cleanup work.
drivers/staging/erofs/internal.h | 11 +++
drivers/staging/erofs/super.c | 5 +
drivers/staging/erofs/unzip_vle.c | 20 ++--
3 files changed, 22 insertions(+), 14 deletions(-)
diff --git a/drivers/staging/erofs/internal.h b/drivers/staging/erofs/internal.h
index cfcc6db..c2cc4a016 100644
--- a/drivers/staging/erofs/internal.h
+++ b/drivers/staging/erofs/internal.h
@@ -95,6 +95,9 @@ struct erofs_sb_info {
/* the dedicated workstation for compression */
struct radix_tree_root workstn_tree;
+ /* threshold for decompression synchronously */
+ unsigned int max_sync_decompress_pages;
+
#ifdef EROFS_FS_HAS_MANAGED_CACHE
struct inode *managed_cache;
#endif
@@ -273,6 +276,14 @@ extern int erofs_try_to_free_cached_page(struct
address_space *mapping,
struct page *page);
#endif
+#define DEFAULT_MAX_SYNC_DECOMPRESS_PAGES 3
+
+static inline bool __should_decompress_synchronously(struct erofs_sb_info *sbi,
+unsigned int nr)
+{
+ return nr <= sbi->max_sync_decompress_pages;
+}
+
#endif
/* we strictly follow PAGE_SIZE and no buffer head yet */
diff --git a/drivers/staging/erofs/super.c b/drivers/staging/erofs/super.c
index 802202c..5b87f59 100644
--- a/drivers/staging/erofs/super.c
+++ b/drivers/staging/erofs/super.c
@@ -162,6 +162,11 @@ static void erofs_build_fault_attr(struct erofs_sb_info
*sbi,
static void default_options(struct erofs_sb_info *sbi)
{
+ /* set up some FS parameters */
+#ifdef CONFIG_EROFS_FS_ZIP
+ sbi->max_sync_decompress_pages = DEFAULT_MAX_SYNC_DECOMPRESS_PAGES;
+#endif
+
#ifdef CONFIG_EROFS_FS_XATTR
set_opt(sbi, XATTR_USER);
#endif
diff --git a/drivers/staging/erofs/unzip_vle.c
b/drivers/staging/erofs/unzip_vle.c
index 9fe18cd3..337a82d 100644
--- a/drivers/staging/erofs/unzip_vle.c
+++ b/drivers/staging/erofs/unzip_vle.c
@@ -1308,12 +1308,14 @@ static int z_erofs_vle_normalaccess_readpage(struct
file *file,
return 0;
}
-static inline int __z_erofs_vle_normalaccess_readpages(
- struct file *filp,
- struct address_space *mapping,
- struct list_head *pages, unsigned int nr_pages, bool sync)
+static int z_erofs_vle_normalaccess_readpages(struct file *filp,
+ struct address_space *mapping,
+ struct list_head *pages,
+ unsigned int nr_pages)
{
struct inode *const inode = mapping->host;
+ struct erofs_sb_info *const sbi = EROFS_I_SB(inode);
+ const bool sync = __should_decompress_synchronously(sbi, nr_pages);
struct z_erofs_vle_frontend f = VLE_FRONTEND_INIT(inode);
gfp_t gfp = mapping_gfp_constraint(mapping, GFP_KERNEL);
@@ -1372,16 +1374,6 @@ static inline int __z_erofs_vle_normalaccess_readpages(
return 0;
}
-static int z_erofs_vle_normalaccess_readpages(
- struct file *filp,
- struct address_space *mapping,
- struct list_head *pages, unsigned int nr_pages)
-{
- return __z_erofs_vle_normalaccess_readpages(filp,
- mapping, pages, nr_pages,
- nr_pages < 4 /* sync */);
-}
-
const struct address_space_operations z_erofs_vle_normalaccess_aops = {
.readpage = z_erofs_vle_normalaccess_readpage,
.readpages = z_erofs_vle_normalaccess_readpages,
--
2.7.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: erofs: fix undeclared symbols
Hi Thomas,
On 2018/9/21 2:58, Thomas Weißschuh wrote:
> Move all internal symbols to the internal header file and add a missing
> "static" declaration.
> This fixes the sparse warnings like the following:
>
> drivers/staging/erofs/unzip_lz4.c:230:5: warning: symbol 'z_erofs_unzip_lz4'
> was not declared. Should it be static?
>
> Signed-off-by: Thomas Weißschuh
> ---
> drivers/staging/erofs/data.c | 5 -
> drivers/staging/erofs/internal.h | 14 ++
> drivers/staging/erofs/super.c | 5 -
> drivers/staging/erofs/unzip_vle.c | 2 +-
> drivers/staging/erofs/unzip_vle_lz4.c | 2 --
> drivers/staging/erofs/utils.c | 2 --
> 6 files changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/staging/erofs/data.c b/drivers/staging/erofs/data.c
> index ac263a180253..9bfcc549bbf0 100644
> --- a/drivers/staging/erofs/data.c
> +++ b/drivers/staging/erofs/data.c
> @@ -137,11 +137,6 @@ static int erofs_map_blocks_flatmode(struct inode *inode,
> return 0;
> }
>
> -#ifdef CONFIG_EROFS_FS_ZIP
> -extern int z_erofs_map_blocks_iter(struct inode *,
> - struct erofs_map_blocks *, struct page **, int);
> -#endif
> -
> int erofs_map_blocks_iter(struct inode *inode,
> struct erofs_map_blocks *map,
> struct page **mpage_ret, int flags)
> diff --git a/drivers/staging/erofs/internal.h
> b/drivers/staging/erofs/internal.h
> index 367b39fe46e5..d4c4c87bcd35 100644
> --- a/drivers/staging/erofs/internal.h
> +++ b/drivers/staging/erofs/internal.h
> @@ -547,6 +547,20 @@ extern unsigned long erofs_shrink_count(struct shrinker
> *shrink,
> struct shrink_control *sc);
> extern unsigned long erofs_shrink_scan(struct shrinker *shrink,
> struct shrink_control *sc);
> +extern void erofs_workgroup_free_rcu(struct erofs_workgroup *grp);
> +
> +#ifdef CONFIG_EROFS_FS_ZIP
> +/* super.c */
> +extern int z_erofs_init_zip_subsystem(void);
> +extern void z_erofs_exit_zip_subsystem(void);
> +
> +/* unzip_vle.c */
> +extern int z_erofs_map_blocks_iter(struct inode *,
> + struct erofs_map_blocks *, struct page **, int);
> +
> +/* unzip_lz4.c */
> +extern int z_erofs_unzip_lz4(void *in, void *out, size_t inlen, size_t
> outlen);
Thanks for your patch.
Here z_erofs_unzip_lz4 couldn't be directly declared in internal.h
--- internal.h means the file system internal but not for the decompression
algorithms.
Some declarations in *.c are for temporary use.
z_erofs_unzip_lz4 has no related with the file system itself and I planned to
cleanup later
after we have more decompression algorithm support such as zstd
If you want to cleanup now, prefer to introduce "decompressor wrappers" and a
new .h
rather than cleanup as simple as what is done in this commit.
> +#endif
>
> #ifndef lru_to_page
> #define lru_to_page(head) (list_entry((head)->prev, struct page, lru))
> diff --git a/drivers/staging/erofs/super.c b/drivers/staging/erofs/super.c
> index 2df9768edac9..c41d92e0cb3c 100644
> --- a/drivers/staging/erofs/super.c
> +++ b/drivers/staging/erofs/super.c
> @@ -521,11 +521,6 @@ static struct file_system_type erofs_fs_type = {
> };
> MODULE_ALIAS_FS("erofs");
>
> -#ifdef CONFIG_EROFS_FS_ZIP
> -extern int z_erofs_init_zip_subsystem(void);
> -extern void z_erofs_exit_zip_subsystem(void);
> -#endif
> -
> static int __init erofs_module_init(void)
> {
> int err;
> diff --git a/drivers/staging/erofs/unzip_vle.c
> b/drivers/staging/erofs/unzip_vle.c
> index 8721f0a41d15..a0d6c620051f 100644
> --- a/drivers/staging/erofs/unzip_vle.c
> +++ b/drivers/staging/erofs/unzip_vle.c
> @@ -517,7 +517,7 @@ static void __z_erofs_vle_work_release(struct
> z_erofs_vle_workgroup *grp,
> erofs_workgroup_put(&grp->obj);
> }
>
> -void z_erofs_vle_work_release(struct z_erofs_vle_work *work)
> +static void z_erofs_vle_work_release(struct z_erofs_vle_work *work)
> {
> struct z_erofs_vle_workgroup *grp =
> z_erofs_vle_work_workgroup(work, true);
How about making a separate patch to fix all the missing `static's?
Or How about changing your patch title "staging: erofs: fix undeclared symbols"
to indicate you also add some missing `static's ?
Thanks,
Gao Xiang
> diff --git a/drivers/staging/erofs/unzip_vle_lz4.c
> b/drivers/staging/erofs/unzip_vle_lz4.c
> index f5b665f15be5..e30e6e2ef05b 100644
> --- a/drivers/staging/erofs/unzip_vle_lz4.c
> +++ b/drivers/staging/erofs/unzip_vle_lz4.c
> @@ -99,8 +99,6 @@ int z_erofs_vle_plain_copy(struct page **compressed_pages,
> return 0;
> }
>
> -extern int z_erofs_unzip_lz4(void *in, void *out, s
Re: [PATCH] staging: erofs: change inode related info in erofs_statfs()
Hi Chengguang, On 2018/9/25 7:41, Chengguang Xu wrote: > As a read only filesystem, it's better to show available > inode num as 0 in statfs. > > Signed-off-by: Chengguang Xu > --- > drivers/staging/erofs/super.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/staging/erofs/super.c b/drivers/staging/erofs/super.c > index 51b076255988..6601a242071f 100644 > --- a/drivers/staging/erofs/super.c > +++ b/drivers/staging/erofs/super.c > @@ -627,8 +627,8 @@ static int erofs_statfs(struct dentry *dentry, struct > kstatfs *buf) > buf->f_blocks = sbi->blocks; > buf->f_bfree = buf->f_bavail = 0; > > - buf->f_files = ULLONG_MAX; > - buf->f_ffree = ULLONG_MAX - sbi->inos; > + buf->f_files = sbi->inos; For erofs, nid indicates the inode position rather than just a id, and it could not be continious. The in-byte inode position is calculated by the following formula: nid * 32(inode_v1) + meta_blkaddr * 4096 These two fields are defined as: fsfilcnt_tf_filesTotal number of file serial numbers. fsfilcnt_tf_ffreeTotal number of free file serial numbers. I'm afraid if f_files == sbi->inos, the actual inode number (nid) could be larger than f_files. I have no idea whether it could give undefined behavior to user program... Thanks, Gao Xiang > + buf->f_ffree = 0; > > buf->f_namelen = EROFS_NAME_LEN; > > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: erofs: fix indenting to conform to kernel coding style
Hi Ioannis,
Thanks for your patch.
Could you please resend this patch to the staging-devel/erofs-devel mailing
list?
Since we are in the China National Day holiday and I can only send email in
the corporation by the HUAWEI mailbox, I cannot reply emails in time...
It could be better to Cc patches to the proper mailing lists...
On 2018/10/6 1:23, Ioannis Valasakis wrote:
> Cleanup all inconsistent indenting to conform to kernel coding style.
> Reported by checkpatch.
It should be fixed, but out of curiousity, could you fix all
the indentings of erofs in this patch?
>
> Signed-off-by: Ioannis Valasakis
> ---
> drivers/staging/erofs/data.c | 56 ++--
> 1 file changed, 28 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/staging/erofs/data.c b/drivers/staging/erofs/data.c
> index 6384f73e5418..ffd392e74803 100644
> --- a/drivers/staging/erofs/data.c
> +++ b/drivers/staging/erofs/data.c
> @@ -40,7 +40,7 @@ static inline void read_endio(struct bio *bio)
>
> /* prio -- true is used for dir */
> struct page *__erofs_get_meta_page(struct super_block *sb,
> - erofs_blk_t blkaddr, bool prio, bool nofail)
> +erofs_blk_t blkaddr, bool prio, bool nofail)
> {
> struct inode *const bd_inode = sb->s_bdev->bd_inode;
> struct address_space *const mapping = bd_inode->i_mapping;
> @@ -53,7 +53,7 @@ struct page *__erofs_get_meta_page(struct super_block *sb,
>
> repeat:
> page = find_or_create_page(mapping, blkaddr, gfp);
> - if (unlikely(page == NULL)) {
> + if (unlikely(!page)) {
Here should be fixed too, but it is another case.
Could you make another patch to fix the NULL pointer checks?
Thanks,
Gao Xiang
> DBG_BUGON(nofail);
> return ERR_PTR(-ENOMEM);
> }
> @@ -76,7 +76,7 @@ struct page *__erofs_get_meta_page(struct super_block *sb,
> }
>
> __submit_bio(bio, REQ_OP_READ,
> - REQ_META | (prio ? REQ_PRIO : 0));
> + REQ_META | (prio ? REQ_PRIO : 0));
>
> lock_page(page);
>
> @@ -107,7 +107,7 @@ struct page *__erofs_get_meta_page(struct super_block *sb,
> }
>
> static int erofs_map_blocks_flatmode(struct inode *inode,
> - struct erofs_map_blocks *map,
> + struct erofs_map_blocks *map,
> int flags)
> {
> int err = 0;
> @@ -151,7 +151,7 @@ static int erofs_map_blocks_flatmode(struct inode *inode,
> map->m_flags |= EROFS_MAP_META;
> } else {
> errln("internal error @ nid: %llu (size %llu), m_la 0x%llx",
> - vi->nid, inode->i_size, map->m_la);
> + vi->nid, inode->i_size, map->m_la);
> DBG_BUGON(1);
> err = -EIO;
> goto err_out;
> @@ -167,16 +167,17 @@ static int erofs_map_blocks_flatmode(struct inode
> *inode,
>
> #ifdef CONFIG_EROFS_FS_ZIP
> extern int z_erofs_map_blocks_iter(struct inode *,
> - struct erofs_map_blocks *, struct page **, int);
> +struct erofs_map_blocks *,
> +struct page **, int);
> #endif
>
> int erofs_map_blocks_iter(struct inode *inode,
> - struct erofs_map_blocks *map,
> + struct erofs_map_blocks *map,
> struct page **mpage_ret, int flags)
> {
> /* by default, reading raw data never use erofs_map_blocks_iter */
> if (unlikely(!is_inode_layout_compression(inode))) {
> - if (*mpage_ret != NULL)
> + if (*mpage_ret)
> put_page(*mpage_ret);
> *mpage_ret = NULL;
>
> @@ -192,27 +193,27 @@ int erofs_map_blocks_iter(struct inode *inode,
> }
>
> int erofs_map_blocks(struct inode *inode,
> - struct erofs_map_blocks *map, int flags)
> + struct erofs_map_blocks *map, int flags)
> {
> if (unlikely(is_inode_layout_compression(inode))) {
> struct page *mpage = NULL;
> int err;
>
> err = erofs_map_blocks_iter(inode, map, &mpage, flags);
> - if (mpage != NULL)
> + if (mpage)
> put_page(mpage);
> return err;
> }
> return erofs_map_blocks_flatmode(inode, map, flags);
> }
>
> -static inline struct bio *erofs_read_raw_page(
> - struct bio *bio,
> - struct address_space *mapping,
> - struct page *page,
> - erofs_off_t *last_block,
> - unsigned int nblocks,
> - bool ra)
> +static inline struct bio *erofs_read_ra
Re: [PATCH] staging: erofs: add SPDX identifer
Hi Loic, On 2018/10/8 18:41, Loic Tourlonias wrote: > Add SPDX identifier to simplify header and remove whole license text > > Signed-off-by: Loic Tourlonias > --- > drivers/staging/erofs/lz4defs.h | 23 ++- > 1 file changed, 2 insertions(+), 21 deletions(-) > > diff --git a/drivers/staging/erofs/lz4defs.h b/drivers/staging/erofs/lz4defs.h > index 00a0b58a0871..bdb1022981d8 100644 > --- a/drivers/staging/erofs/lz4defs.h > +++ b/drivers/staging/erofs/lz4defs.h > @@ -1,32 +1,13 @@ > #ifndef __LZ4DEFS_H__ > #define __LZ4DEFS_H__ > > +// SPDX-License-Identifier: BSD-2-Clause For *.h files, it seems SPDX Identifier should be /* SPDX-License-Identifier: */ rather than // SPDX-License-Identifier: And I think it should be placed at the first line of the line, but I am not sure of that. Thanks, Gao Xiang > /* > * lz4defs.h -- common and architecture specific defines for the kernel usage > > * LZ4 - Fast LZ compression algorithm > * Copyright (C) 2011-2016, Yann Collet. > - * BSD 2-Clause License (http://www.opensource.org/licenses/bsd-license.php) > - * Redistribution and use in source and binary forms, with or without > - * modification, are permitted provided that the following conditions are > - * met: > - * * Redistributions of source code must retain the above copyright > - * notice, this list of conditions and the following disclaimer. > - * * Redistributions in binary form must reproduce the above > - * copyright notice, this list of conditions and the following disclaimer > - * in the documentation and/or other materials provided with the > - * distribution. > - * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS > - * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT > - * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR > - * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT > - * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, > - * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT > - * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, > - * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY > - * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT > - * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE > - * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > + * > * You can contact the author at : > * - LZ4 homepage : http://www.lz4.org > * - LZ4 source repository : https://github.com/lz4/lz4 > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] staging: erofs: add SPDX identifer
Hi Greg, On 2018/10/8 22:22, Greg Kroah-Hartman wrote: > Why delete that line? > > But wait, why do we even have this file at all? What's wrong with the > lib/lz4/ code that we have in the kernel today? Shouldn't the code > using these files be moved over to use the lib/ code instead and this > file be deleted? EROFS uses customized LZ4 decompression code for now (which has been offically supported in lz4 1.8.3, I have updated it in https://ozlabs.org/~akpm/mmots/broken-out/lib-lz4-update-lz4-decompressor-module.patch if it is shown up in Linux 4.20, I will remove all the customized LZ4 decompression code), but lib/ code lz4def.h isn't export to include/. Thanks, Gao Xiang > > thanks, > > greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] staging: erofs: add SPDX identifer
On 2018/10/8 22:36, Gao Xiang wrote: > Hi Greg, > > On 2018/10/8 22:22, Greg Kroah-Hartman wrote: >> Why delete that line? >> >> But wait, why do we even have this file at all? What's wrong with the >> lib/lz4/ code that we have in the kernel today? Shouldn't the code >> using these files be moved over to use the lib/ code instead and this >> file be deleted? > > EROFS uses customized LZ4 decompression code for now (which has been offically > supported in lz4 1.8.3, I have updated it in > https://ozlabs.org/~akpm/mmots/broken-out/lib-lz4-update-lz4-decompressor-module.patch > if it is shown up in Linux 4.20, I will remove all the customized LZ4 > decompression code), > but lib/ code lz4def.h isn't export to include/. Please refer to https://github.com/lz4/lz4/issues/566 for more details the reason why EROFS uses the different implementation rather than the current lib/lz4/ code in the kernel today (it will be changed in the next Linux version if the updated code works fine.) Thanks, Gao Xiang > > Thanks, > Gao Xiang > >> >> thanks, >> >> greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] staging: erofs: add SPDX identifer
Hi Greg, On 2018/10/8 22:43, Greg Kroah-Hartman wrote: > On Mon, Oct 08, 2018 at 10:36:39PM +0800, Gao Xiang wrote: >> Hi Greg, >> >> On 2018/10/8 22:22, Greg Kroah-Hartman wrote: >>> Why delete that line? >>> >>> But wait, why do we even have this file at all? What's wrong with the >>> lib/lz4/ code that we have in the kernel today? Shouldn't the code >>> using these files be moved over to use the lib/ code instead and this >>> file be deleted? >> >> EROFS uses customized LZ4 decompression code for now (which has been >> offically >> supported in lz4 1.8.3, I have updated it in >> https://ozlabs.org/~akpm/mmots/broken-out/lib-lz4-update-lz4-decompressor-module.patch >> if it is shown up in Linux 4.20, I will remove all the customized LZ4 >> decompression code), >> but lib/ code lz4def.h isn't export to include/. > > It should show up in 4.20, so you should be able to remove this all > then. Yes, that is correct. I will fix it in 4.20 if the updated LZ4 is already there. :) Thanks, Gao Xiang > > thanks, > > greg k-h > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] staging: erofs: add SPDX identifer
Hi Loic, On 2018/10/8 23:00, loïc tourlonias wrote: > Hi Gao, > On Mon, Oct 8, 2018 at 4:48 PM Gao Xiang wrote: >> >> Hi Greg, >> >> On 2018/10/8 22:43, Greg Kroah-Hartman wrote: >>> On Mon, Oct 08, 2018 at 10:36:39PM +0800, Gao Xiang wrote: >>>> Hi Greg, >>>> >>>> On 2018/10/8 22:22, Greg Kroah-Hartman wrote: >>>>> Why delete that line? >>>>> >>>>> But wait, why do we even have this file at all? What's wrong with the >>>>> lib/lz4/ code that we have in the kernel today? Shouldn't the code >>>>> using these files be moved over to use the lib/ code instead and this >>>>> file be deleted? >>>> >>>> EROFS uses customized LZ4 decompression code for now (which has been >>>> offically >>>> supported in lz4 1.8.3, I have updated it in >>>> https://ozlabs.org/~akpm/mmots/broken-out/lib-lz4-update-lz4-decompressor-module.patch >>>> if it is shown up in Linux 4.20, I will remove all the customized LZ4 >>>> decompression code), >>>> but lib/ code lz4def.h isn't export to include/. >>> >>> It should show up in 4.20, so you should be able to remove this all >>> then. >> >> Yes, that is correct. I will fix it in 4.20 if the updated LZ4 is already >> there. :) > > So no need to update the patch since the file will be removed. Am I correct? > > Too bad, I'll make my first patch later... ^^ I think you could make the patch if you want :) It is up to Greg whether take this patch or not since it is actually a cleanup attempt. I will also do more cleanups beside my current work, yet EROFS is now busy in productization. Thanks, Gao Xiang > > Thanks > Loic >> >> Thanks, >> Gao Xiang >> >>> >>> thanks, >>> >>> greg k-h >>> ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: erofs: add the missing __init tags
Append __init to `erofs_init_inode_cache',
`z_erofs_init_zip_subsystem' and move these
declarations to internal.h.
Signed-off-by: Gao Xiang
---
drivers/staging/erofs/internal.h | 6 ++
drivers/staging/erofs/super.c | 13 +
drivers/staging/erofs/unzip_vle.c | 2 +-
3 files changed, 8 insertions(+), 13 deletions(-)
diff --git a/drivers/staging/erofs/internal.h b/drivers/staging/erofs/internal.h
index c2cc4a016d9e..5096b27bcf0d 100644
--- a/drivers/staging/erofs/internal.h
+++ b/drivers/staging/erofs/internal.h
@@ -284,6 +284,12 @@ static inline bool
__should_decompress_synchronously(struct erofs_sb_info *sbi,
return nr <= sbi->max_sync_decompress_pages;
}
+int __init z_erofs_init_zip_subsystem(void);
+void z_erofs_exit_zip_subsystem(void);
+#else
+/* dummy initializer/finalizer for the decompression subsystem */
+static inline int z_erofs_init_zip_subsystem(void) { return 0; }
+static inline void z_erofs_exit_zip_subsystem(void) {}
#endif
/* we strictly follow PAGE_SIZE and no buffer head yet */
diff --git a/drivers/staging/erofs/super.c b/drivers/staging/erofs/super.c
index 51b076255988..f69e619807a1 100644
--- a/drivers/staging/erofs/super.c
+++ b/drivers/staging/erofs/super.c
@@ -29,7 +29,7 @@ static void init_once(void *ptr)
inode_init_once(&vi->vfs_inode);
}
-static int erofs_init_inode_cache(void)
+static int __init erofs_init_inode_cache(void)
{
erofs_inode_cachep = kmem_cache_create("erofs_inode",
sizeof(struct erofs_vnode), 0,
@@ -559,11 +559,6 @@ static struct file_system_type erofs_fs_type = {
};
MODULE_ALIAS_FS("erofs");
-#ifdef CONFIG_EROFS_FS_ZIP
-extern int z_erofs_init_zip_subsystem(void);
-extern void z_erofs_exit_zip_subsystem(void);
-#endif
-
static int __init erofs_module_init(void)
{
int err;
@@ -579,11 +574,9 @@ static int __init erofs_module_init(void)
if (err)
goto shrinker_err;
-#ifdef CONFIG_EROFS_FS_ZIP
err = z_erofs_init_zip_subsystem();
if (err)
goto zip_err;
-#endif
err = register_filesystem(&erofs_fs_type);
if (err)
@@ -593,10 +586,8 @@ static int __init erofs_module_init(void)
return 0;
fs_err:
-#ifdef CONFIG_EROFS_FS_ZIP
z_erofs_exit_zip_subsystem();
zip_err:
-#endif
unregister_shrinker(&erofs_shrinker_info);
shrinker_err:
erofs_exit_inode_cache();
@@ -607,9 +598,7 @@ static int __init erofs_module_init(void)
static void __exit erofs_module_exit(void)
{
unregister_filesystem(&erofs_fs_type);
-#ifdef CONFIG_EROFS_FS_ZIP
z_erofs_exit_zip_subsystem();
-#endif
unregister_shrinker(&erofs_shrinker_info);
erofs_exit_inode_cache();
infoln("successfully finalize erofs");
diff --git a/drivers/staging/erofs/unzip_vle.c
b/drivers/staging/erofs/unzip_vle.c
index 337a82dafa1d..79d3ba62b298 100644
--- a/drivers/staging/erofs/unzip_vle.c
+++ b/drivers/staging/erofs/unzip_vle.c
@@ -42,7 +42,7 @@ static inline int init_unzip_workqueue(void)
return z_erofs_workqueue != NULL ? 0 : -ENOMEM;
}
-int z_erofs_init_zip_subsystem(void)
+int __init z_erofs_init_zip_subsystem(void)
{
z_erofs_workgroup_cachep =
kmem_cache_create("erofs_compress",
--
2.14.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: erofs: harden inode lookup for 32-bit platforms
This patch introduces inode hash function, test and set callbacks,
and iget5_locked to find the right inode for 32-bit platforms.
Reviewed-by: Chao Yu
Signed-off-by: Gao Xiang
---
The patch has been previewed in the linux-erofs mailing list, submit to
the staging mailing list for linux-4.20.
Thanks,
Gao Xiang
drivers/staging/erofs/inode.c| 37 -
drivers/staging/erofs/internal.h | 9 +
2 files changed, 45 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/erofs/inode.c b/drivers/staging/erofs/inode.c
index da8693a7c3d3..04c61a9d7b76 100644
--- a/drivers/staging/erofs/inode.c
+++ b/drivers/staging/erofs/inode.c
@@ -232,10 +232,45 @@ static int fill_inode(struct inode *inode, int isdir)
return err;
}
+/*
+ * erofs nid is 64bits, but i_ino is 'unsigned long', therefore
+ * we should do more for 32-bit platform to find the right inode.
+ */
+#if BITS_PER_LONG == 32
+static int erofs_ilookup_test_actor(struct inode *inode, void *opaque)
+{
+ const erofs_nid_t nid = *(erofs_nid_t *)opaque;
+
+ return EROFS_V(inode)->nid == nid;
+}
+
+static int erofs_iget_set_actor(struct inode *inode, void *opaque)
+{
+ const erofs_nid_t nid = *(erofs_nid_t *)opaque;
+
+ inode->i_ino = erofs_inode_hash(nid);
+ return 0;
+}
+#endif
+
+static inline struct inode *erofs_iget_locked(struct super_block *sb,
+ erofs_nid_t nid)
+{
+ const unsigned long hashval = erofs_inode_hash(nid);
+
+#if BITS_PER_LONG >= 64
+ /* it is safe to use iget_locked for >= 64-bit platform */
+ return iget_locked(sb, hashval);
+#else
+ return iget5_locked(sb, hashval, erofs_ilookup_test_actor,
+ erofs_iget_set_actor, &nid);
+#endif
+}
+
struct inode *erofs_iget(struct super_block *sb,
erofs_nid_t nid, bool isdir)
{
- struct inode *inode = iget_locked(sb, nid);
+ struct inode *inode = erofs_iget_locked(sb, nid);
if (unlikely(inode == NULL))
return ERR_PTR(-ENOMEM);
diff --git a/drivers/staging/erofs/internal.h b/drivers/staging/erofs/internal.h
index 5096b27bcf0d..57575c7f5635 100644
--- a/drivers/staging/erofs/internal.h
+++ b/drivers/staging/erofs/internal.h
@@ -517,6 +517,15 @@ erofs_get_inline_page(struct inode *inode,
}
/* inode.c */
+static inline unsigned long erofs_inode_hash(erofs_nid_t nid)
+{
+#if BITS_PER_LONG == 32
+ return (nid >> 32) ^ (nid & 0x);
+#else
+ return nid;
+#endif
+}
+
extern struct inode *erofs_iget(struct super_block *sb,
erofs_nid_t nid, bool dir);
--
2.14.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: erofs: harden inode lookup for 32-bit platforms
On 2018/10/11 16:44, Dan Carpenter wrote: > On Tue, Oct 09, 2018 at 10:07:13PM +0800, Gao Xiang wrote: >> This patch introduces inode hash function, test and set callbacks, >> and iget5_locked to find the right inode for 32-bit platforms. >> > > The way I read this changelog, we're trying to deal with corrupt file > systems? Is that correct? Presumably in the current code it could lead > to a Oops or something? No, this commit isn't trying to deal with corrupt file systems. In EROFS, the nid is not continuous and it represents the inode offset inode offset = nid * 32. Therefore the nid is 64-bit both for 32-bit and 64-bit platforms. However, i_ino is 'unsigned long', which means for 32-bit platforms, i_ino is not enough to contain the nid. Therefore, we should use iget5_locked for this case. > > regards, > dan carpenter > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: erofs: harden inode lookup for 32-bit platforms
Hi Dan, On 2018/10/11 18:18, Dan Carpenter wrote: > On Thu, Oct 11, 2018 at 05:46:26PM +0800, Gao Xiang wrote: >> >> >> On 2018/10/11 16:44, Dan Carpenter wrote: >>> On Tue, Oct 09, 2018 at 10:07:13PM +0800, Gao Xiang wrote: >>>> This patch introduces inode hash function, test and set callbacks, >>>> and iget5_locked to find the right inode for 32-bit platforms. >>>> >>> >>> The way I read this changelog, we're trying to deal with corrupt file >>> systems? Is that correct? Presumably in the current code it could lead >>> to a Oops or something? >> >> No, this commit isn't trying to deal with corrupt file systems. >> In EROFS, the nid is not continuous and it represents the inode offset >> inode offset = nid * 32. >> Therefore the nid is 64-bit both for 32-bit and 64-bit platforms. However, >> i_ino is 'unsigned long', which means for 32-bit platforms, i_ino is not >> enough >> to contain the nid. >> >> Therefore, we should use iget5_locked for this case. > > I guess what I'm saying is, what are the user visible effects of this > patch? It's hard for me to tell from the patch description. Could you > please re-write the description and send a v2? Sorry for my just hurry reply. It seems I really misunderstood your point :-( If all nids are less than 32-bit, the user will see the correct inode/nid number for all 32-bit platforms, but if some nids are greater than 32-bit, it could be collisions without this patch for 32-bit platforms. This patch switch the inode number by XOR the high 32-bit and the low 32-bit, use customized tester to avoid collisions for 32-bit platforms. I will send v2 to fix the commit message...but it seems Greg is already merged in https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/commit/?h=staging-testing&id=2abd7814138774eb56319e9dcc0abd08ece45424 Thanks, Gao Xiang > > regards, > dan carpenter > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: erofs: harden inode lookup for 32-bit platforms
Hi Dan,
On 2018/10/11 19:12, Dan Carpenter wrote:
> On Thu, Oct 11, 2018 at 06:49:57PM +0800, Gao Xiang wrote:
>> Hi Dan,
>>
>> On 2018/10/11 18:18, Dan Carpenter wrote:
>>> On Thu, Oct 11, 2018 at 05:46:26PM +0800, Gao Xiang wrote:
>>>>
>>>>
>>>> On 2018/10/11 16:44, Dan Carpenter wrote:
>>>>> On Tue, Oct 09, 2018 at 10:07:13PM +0800, Gao Xiang wrote:
>>>>>> This patch introduces inode hash function, test and set callbacks,
>>>>>> and iget5_locked to find the right inode for 32-bit platforms.
>>>>>>
>>>>>
>>>>> The way I read this changelog, we're trying to deal with corrupt file
>>>>> systems? Is that correct? Presumably in the current code it could lead
>>>>> to a Oops or something?
>>>>
>>>> No, this commit isn't trying to deal with corrupt file systems.
>>>> In EROFS, the nid is not continuous and it represents the inode offset
>>>> inode offset = nid * 32.
>>>> Therefore the nid is 64-bit both for 32-bit and 64-bit platforms. However,
>>>> i_ino is 'unsigned long', which means for 32-bit platforms, i_ino is not
>>>> enough
>>>> to contain the nid.
>>>>
>>>> Therefore, we should use iget5_locked for this case.
>>>
>>> I guess what I'm saying is, what are the user visible effects of this
>>> patch? It's hard for me to tell from the patch description. Could you
>>> please re-write the description and send a v2?
>>
>> Sorry for my just hurry reply. It seems I really misunderstood your point :-(
>>
>> If all nids are less than 32-bit, the user will see the correct inode/nid
>> number
>> for all 32-bit platforms, but if some nids are greater than 32-bit, it could
>> be
>> collisions without this patch for 32-bit platforms.
>>
>> This patch switch the inode number by XOR the high 32-bit and the low 32-bit,
>> use customized tester to avoid collisions for 32-bit platforms.
>>
>> I will send v2 to fix the commit message...but it seems Greg is already
>> merged in
>> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/commit/?h=staging-testing&id=2abd7814138774eb56319e9dcc0abd08ece45424
>>
>
> Oh. Then that's fine then. If Greg already merged it, then it's too
> late to change. Thanks for the explanation, though.
I appended the v2 patch in the end of this reply for reference...
Thanks,
Gao Xiang
>
> regards,
> dan carpenter
>
>From 175eb3f1c6e1cddf8292853ab0fb7644320c3ae2 Mon Sep 17 00:00:00 2001
From: Gao Xiang
Date: Mon, 1 Oct 2018 12:32:55 +0800
Subject: [PATCH v2] staging: erofs: harden inode lookup for 32-bit platforms
Each erofs inode has a unique 64-bit nid to distinguish from others.
However, the inode number is usually defined as `unsigned long',
which means some collisions could happen if we directly use iget_locked
for 32-bit platforms.
Therefore, this patch introduces a inode hash function, test and set
callbacks, and iget5_locked to avoid potential collisions
for 32-bit platforms.
In addition, if the value of a nid is greater than 32-bit, users from
32-bit platforms will see XOR-ed inode numbers after this patch.
Reviewed-by: Chao Yu
Signed-off-by: Gao Xiang
---
change log v2:
- add more infos to the commit message as pointed out by Dan Carpenter.
Thanks,
Gao Xiang
drivers/staging/erofs/inode.c| 37 -
drivers/staging/erofs/internal.h | 9 +
2 files changed, 45 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/erofs/inode.c b/drivers/staging/erofs/inode.c
index da8693a7c3d3..04c61a9d7b76 100644
--- a/drivers/staging/erofs/inode.c
+++ b/drivers/staging/erofs/inode.c
@@ -232,10 +232,45 @@ static int fill_inode(struct inode *inode, int isdir)
return err;
}
+/*
+ * erofs nid is 64bits, but i_ino is 'unsigned long', therefore
+ * we should do more for 32-bit platform to find the right inode.
+ */
+#if BITS_PER_LONG == 32
+static int erofs_ilookup_test_actor(struct inode *inode, void *opaque)
+{
+ const erofs_nid_t nid = *(erofs_nid_t *)opaque;
+
+ return EROFS_V(inode)->nid == nid;
+}
+
+static int erofs_iget_set_actor(struct inode *inode, void *opaque)
+{
+ const erofs_nid_t nid = *(erofs_nid_t *)opaque;
+
+ inode->i_ino = erofs_inode_hash(nid);
+ return 0;
+}
+#endif
+
+static inline struct inode *erofs_iget_locked(struct super_block *sb,
+ erofs_nid_t nid)
+{
+ const unsigned long hashval = erofs_inode_hash(nid);
+
+#if BITS_PER_LONG >= 64
+ /* it is safe to use iget_lo
[PATCH RESEND] staging: erofs: clean erofs_lookup()
From: Al Viro
d_splice_alias() does the right thing when given
ERR_PTR(-E...) for inode. No need for gotos, multiple
returns, etc. in there.
Signed-off-by: Al Viro
Reviewed-by: Gao Xiang
Signed-off-by: Gao Xiang
---
Hi,
Frankly, I think it is a straight-forward cleanup by Al enough
to be submitted to Linux 4.20 in constant to other pending fixes
found in the process of EROFS productization these days, which
I need more time to think over and fix formally to the community.
p.s. I have no idea whether this patch has been already queued up
in Al's fs tree for 4.20... :'( +Cc Greg / the staging mailing
list as well. Or please ignore this email...
Thanks,
Gao Xiang
drivers/staging/erofs/namei.c | 19 +++
1 file changed, 7 insertions(+), 12 deletions(-)
diff --git a/drivers/staging/erofs/namei.c b/drivers/staging/erofs/namei.c
index 0039b76..5596c52 100644
--- a/drivers/staging/erofs/namei.c
+++ b/drivers/staging/erofs/namei.c
@@ -223,18 +223,13 @@ static struct dentry *erofs_lookup(struct inode *dir,
if (err == -ENOENT) {
/* negative dentry */
inode = NULL;
- goto negative_out;
- } else if (unlikely(err))
- return ERR_PTR(err);
-
- debugln("%s, %s (nid %llu) found, d_type %u", __func__,
- dentry->d_name.name, nid, d_type);
-
- inode = erofs_iget(dir->i_sb, nid, d_type == EROFS_FT_DIR);
- if (IS_ERR(inode))
- return ERR_CAST(inode);
-
-negative_out:
+ } else if (unlikely(err)) {
+ inode = ERR_PTR(err);
+ } else {
+ debugln("%s, %s (nid %llu) found, d_type %u", __func__,
+ dentry->d_name.name, nid, d_type);
+ inode = erofs_iget(dir->i_sb, nid, d_type == EROFS_FT_DIR);
+ }
return d_splice_alias(inode, dentry);
}
--
2.7.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH RESEND] staging: erofs: clean erofs_lookup()
Hi,
Please ignore this patch, it has been queued by Al for 4.20 in
https://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git/commit/?h=for-next&id=8300807f9e2dffb6552514763ad60e005c12eb94
Sorry for bothering...
Thanks,
Gao Xiang
On 2018/10/13 1:59, Gao Xiang via Linux-erofs wrote:
> From: Al Viro
>
> d_splice_alias() does the right thing when given
> ERR_PTR(-E...) for inode. No need for gotos, multiple
> returns, etc. in there.
>
> Signed-off-by: Al Viro
> Reviewed-by: Gao Xiang
> Signed-off-by: Gao Xiang
> ---
> Hi,
>
> Frankly, I think it is a straight-forward cleanup by Al enough
> to be submitted to Linux 4.20 in constant to other pending fixes
> found in the process of EROFS productization these days, which
> I need more time to think over and fix formally to the community.
>
> p.s. I have no idea whether this patch has been already queued up
> in Al's fs tree for 4.20... :'( +Cc Greg / the staging mailing
> list as well. Or please ignore this email...
>
> Thanks,
> Gao Xiang
>
> drivers/staging/erofs/namei.c | 19 +++
> 1 file changed, 7 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/staging/erofs/namei.c b/drivers/staging/erofs/namei.c
> index 0039b76..5596c52 100644
> --- a/drivers/staging/erofs/namei.c
> +++ b/drivers/staging/erofs/namei.c
> @@ -223,18 +223,13 @@ static struct dentry *erofs_lookup(struct inode *dir,
> if (err == -ENOENT) {
> /* negative dentry */
> inode = NULL;
> - goto negative_out;
> - } else if (unlikely(err))
> - return ERR_PTR(err);
> -
> - debugln("%s, %s (nid %llu) found, d_type %u", __func__,
> - dentry->d_name.name, nid, d_type);
> -
> - inode = erofs_iget(dir->i_sb, nid, d_type == EROFS_FT_DIR);
> - if (IS_ERR(inode))
> - return ERR_CAST(inode);
> -
> -negative_out:
> + } else if (unlikely(err)) {
> + inode = ERR_PTR(err);
> + } else {
> + debugln("%s, %s (nid %llu) found, d_type %u", __func__,
> + dentry->d_name.name, nid, d_type);
> + inode = erofs_iget(dir->i_sb, nid, d_type == EROFS_FT_DIR);
> + }
> return d_splice_alias(inode, dentry);
> }
>
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: erofs: remove the redundant d_rehash() for the root dentry
There is actually no need at all to d_rehash() for the root dentry
as Al pointed out, fix it.
Reported-by: Al Viro
Cc: Al Viro
Signed-off-by: Gao Xiang
---
drivers/staging/erofs/super.c | 6 --
1 file changed, 6 deletions(-)
diff --git a/drivers/staging/erofs/super.c b/drivers/staging/erofs/super.c
index f69e619807a1..1ab3553c839b 100644
--- a/drivers/staging/erofs/super.c
+++ b/drivers/staging/erofs/super.c
@@ -442,12 +442,6 @@ static int erofs_read_super(struct super_block *sb,
erofs_register_super(sb);
- /*
-* We already have a positive dentry, which was instantiated
-* by d_make_root. Just need to d_rehash it.
-*/
- d_rehash(sb->s_root);
-
if (!silent)
infoln("mounted on %s with opts: %s.", dev_name,
(char *)data);
--
2.14.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: erofs: use the new LZ4_decompress_safe_partial()
LZ4_decompress_safe_partial() can now decode exactly the nb of bytes
requested after the upstreamed commit 2209fda323e2 ("lib/lz4: update LZ4
decompressor module"), remove the erofs hacked lz4 decompression code.
A more cleaned compressor wrapper will be introduced in the subsequent
patches in order to prepare for supporting multiple compression algorithms.
Signed-off-by: Gao Xiang
---
Hi Chao,
Could you please kindly confirm (review) this before merging into the staging
tree?
Thanks,
Gao Xiang
drivers/staging/erofs/Makefile| 2 +-
drivers/staging/erofs/lz4defs.h | 227 --
drivers/staging/erofs/unzip_lz4.c | 251 --
drivers/staging/erofs/unzip_vle_lz4.c | 25 +++-
4 files changed, 24 insertions(+), 481 deletions(-)
delete mode 100644 drivers/staging/erofs/lz4defs.h
delete mode 100644 drivers/staging/erofs/unzip_lz4.c
diff --git a/drivers/staging/erofs/Makefile b/drivers/staging/erofs/Makefile
index 9a766eb7ed75..c91b65223f99 100644
--- a/drivers/staging/erofs/Makefile
+++ b/drivers/staging/erofs/Makefile
@@ -9,5 +9,5 @@ obj-$(CONFIG_EROFS_FS) += erofs.o
ccflags-y += -I$(src)/include
erofs-objs := super.o inode.o data.o namei.o dir.o utils.o
erofs-$(CONFIG_EROFS_FS_XATTR) += xattr.o
-erofs-$(CONFIG_EROFS_FS_ZIP) += unzip_vle.o unzip_lz4.o unzip_vle_lz4.o
+erofs-$(CONFIG_EROFS_FS_ZIP) += unzip_vle.o unzip_vle_lz4.o
diff --git a/drivers/staging/erofs/lz4defs.h b/drivers/staging/erofs/lz4defs.h
deleted file mode 100644
index 00a0b58a0871..
--- a/drivers/staging/erofs/lz4defs.h
+++ /dev/null
@@ -1,227 +0,0 @@
-#ifndef __LZ4DEFS_H__
-#define __LZ4DEFS_H__
-
-/*
- * lz4defs.h -- common and architecture specific defines for the kernel usage
-
- * LZ4 - Fast LZ compression algorithm
- * Copyright (C) 2011-2016, Yann Collet.
- * BSD 2-Clause License (http://www.opensource.org/licenses/bsd-license.php)
- * Redistribution and use in source and binary forms, with or without
- * modification, are permitted provided that the following conditions are
- * met:
- * * Redistributions of source code must retain the above copyright
- * notice, this list of conditions and the following disclaimer.
- * * Redistributions in binary form must reproduce the above
- * copyright notice, this list of conditions and the following disclaimer
- * in the documentation and/or other materials provided with the
- * distribution.
- * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
- * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
- * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
- * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
- * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
- * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
- * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
- * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
- * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
- * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
- * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
- * You can contact the author at :
- * - LZ4 homepage : http://www.lz4.org
- * - LZ4 source repository : https://github.com/lz4/lz4
- *
- * Changed for kernel usage by:
- * Sven Schmidt <4ssch...@informatik.uni-hamburg.de>
- */
-
-#include
-#include/* memset, memcpy */
-
-#define FORCE_INLINE __always_inline
-
-/*-
- * Basic Types
- **/
-#include
-
-typedefuint8_t BYTE;
-typedef uint16_t U16;
-typedef uint32_t U32;
-typedefint32_t S32;
-typedef uint64_t U64;
-typedef uintptr_t uptrval;
-
-/*-
- * Architecture specifics
- **/
-#if defined(CONFIG_64BIT)
-#define LZ4_ARCH64 1
-#else
-#define LZ4_ARCH64 0
-#endif
-
-#if defined(__LITTLE_ENDIAN)
-#define LZ4_LITTLE_ENDIAN 1
-#else
-#define LZ4_LITTLE_ENDIAN 0
-#endif
-
-/*-
- * Constants
- **/
-#define MINMATCH 4
-
-#define WILDCOPYLENGTH 8
-#define LASTLITERALS 5
-#define MFLIMIT (WILDCOPYLENGTH + MINMATCH)
-
-/* Increase this value ==> compression run slower on incompressible data */
-#define LZ4_SKIPTRIGGER 6
-
-#define HASH_UNIT sizeof(size_t)
-
-#define KB (1 << 10)
-#define MB (1 << 20)
-#define GB (1U << 30)
-
-#define MAXD_LOG 16
-#define MAX_DISTANCE ((1 << MAXD_LOG) - 1)
-#define STEPSIZE sizeof(size_t)
-
-#define ML_BITS4
-#define ML_MASK((1U << ML_BITS) - 1)
-#define RUN_BITS (8 - ML_BITS)
-#define RUN_MASK ((1U << RUN_BITS) - 1)
-
-/*-
- * Reading and writing into memo
Re: [PATCH] staging: erofs: use the new LZ4_decompress_safe_partial()
Hi Chao,
On 2018/11/8 14:11, Chao Yu wrote:
> On 2018/11/8 12:00, Gao Xiang wrote:
>> LZ4_decompress_safe_partial() can now decode exactly the nb of bytes
>> requested after the upstreamed commit 2209fda323e2 ("lib/lz4: update LZ4
>> decompressor module"), remove the erofs hacked lz4 decompression code.
>>
>> A more cleaned compressor wrapper will be introduced in the subsequent
>> patches in order to prepare for supporting multiple compression algorithms.
>>
>> Signed-off-by: Gao Xiang
>> ---
>>
>> Hi Chao,
>> Could you please kindly confirm (review) this before merging into the
>> staging tree?
>
> Looks good to me.
>
> Reviewed-by: Chao Yu
>
Thanks for quickly confirming this :)
Thanks,
Gao Xiang
> Thanks,
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: How can I get user space tools of erofs?
Hi Chengguang, Good question! It's in the final stage of preparation to open source erofs-mkfs (actually they are now struggle at how to properly spilt into reasonable patches this week), hopefully the implementation could be released at the next week. (sorry I didn't mean to delay, I have to put it in the first place --- successfully launch our linux-erofs products to the market) @Guifu Li is the original author of erofs-mkfs, he will post the original mkfs source code to linux-erofs mailing list and he will maintain erofs-mkfs together with @Wei Fang later. You can contact them for further informations. --- these piece of code is actually not clean enough (a lot hacked/dirty code compared to the kernel code) so a lot of cleanup will be done then. currently, you can get erofs-mkfs binary from (still sorry to say that...): https://github.com/hsiangkao/erofs_mkfs_binary erofs is now in productization for these months, if all things go well, you'll see that HUAWEI mobile phones on the market run in erofs in few months. :) These months I'm busy in solving bugs found by internal beta users and tuning memory policy in heavy memory workload for the best performance compared to ext4 (we have native in-place decompression compared with squashfs/btrfs, thus less extra memory allocation results in lower memory memory reclaim / page-writeback for devices with limited memory, see: https://lists.ozlabs.org/pipermail/linux-erofs/2018-August/000494.html) in order to gain the competitive user experience comparing to uncompressed filesystem solutions. I will update a document to describe our core design and linux-erofs future roadmap in this linux-4.21 round. Thanks, Gao Xiang On 2018/11/9 16:37, cgxu519 wrote: > Hi Xiang, > > Could I ask a simple question about where can I get user space tools(like > mkfs) of erofs? > > > Thanks. > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: erofs: fix undefined LZ4_decompress_safe_partial()
It needs an explicit LZ4 library dependency
if lz4 compression is enabled, found by kbuild randconfig.
Reported-by: kbuild test robot
Fixes: 05f9d4a0c8c4 ("staging: erofs: use the new
LZ4_decompress_safe_partial()")
Signed-off-by: Gao Xiang
---
drivers/staging/erofs/Kconfig | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/erofs/Kconfig b/drivers/staging/erofs/Kconfig
index c8521d7..d04b798 100644
--- a/drivers/staging/erofs/Kconfig
+++ b/drivers/staging/erofs/Kconfig
@@ -90,8 +90,9 @@ config EROFS_FS_IO_MAX_RETRIES
config EROFS_FS_ZIP
bool "EROFS Data Compresssion Support"
depends on EROFS_FS
+ select LZ4_DECOMPRESS
help
- Currently we support VLE Compression only.
+ Currently we support LZ4 VLE Compression only.
Play at your own risk.
If you don't want to use compression feature, say N.
--
2.7.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/3] staging: erofs: unzip_vle.c: Align parameter to the parentesis
On 2018/11/13 4:43, Cristian Sicilia wrote:
> Align parameters to the opened parentesis.
>
> Signed-off-by: Cristian Sicilia
Reviewed-by: Gao Xiang
Thanks,
Gao Xiang
> ---
> drivers/staging/erofs/unzip_vle.c | 11 ++-
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/staging/erofs/unzip_vle.c
> b/drivers/staging/erofs/unzip_vle.c
> index 35add4e..6a283f6 100644
> --- a/drivers/staging/erofs/unzip_vle.c
> +++ b/drivers/staging/erofs/unzip_vle.c
> @@ -35,9 +35,10 @@ static inline int init_unzip_workqueue(void)
>* we don't need too many threads, limiting threads
>* could improve scheduling performance.
>*/
> - z_erofs_workqueue = alloc_workqueue("erofs_unzipd",
> - WQ_UNBOUND | WQ_HIGHPRI | WQ_CPU_INTENSIVE,
> - onlinecpus + onlinecpus / 4);
> + z_erofs_workqueue =
> + alloc_workqueue("erofs_unzipd",
> + WQ_UNBOUND | WQ_HIGHPRI | WQ_CPU_INTENSIVE,
> + onlinecpus + onlinecpus / 4);
>
> return z_erofs_workqueue ? 0 : -ENOMEM;
> }
> @@ -46,8 +47,8 @@ int __init z_erofs_init_zip_subsystem(void)
> {
> z_erofs_workgroup_cachep =
> kmem_cache_create("erofs_compress",
> - Z_EROFS_WORKGROUP_SIZE, 0,
> - SLAB_RECLAIM_ACCOUNT, NULL);
> + Z_EROFS_WORKGROUP_SIZE, 0,
> + SLAB_RECLAIM_ACCOUNT, NULL);
>
> if (z_erofs_workgroup_cachep) {
> if (!init_unzip_workqueue())
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/3] staging: erofs: unzip_vle.c: Constant in comparison on right side
On 2018/11/13 4:43, Cristian Sicilia wrote:
> Comparisons should place the constant
> on the right side of the test.
>
> Signed-off-by: Cristian Sicilia
Reviewed-by: Gao Xiang
Thanks,
Gao Xiang
> ---
> drivers/staging/erofs/unzip_vle.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/erofs/unzip_vle.c
> b/drivers/staging/erofs/unzip_vle.c
> index 1ffeeaa..35add4e 100644
> --- a/drivers/staging/erofs/unzip_vle.c
> +++ b/drivers/staging/erofs/unzip_vle.c
> @@ -250,8 +250,8 @@ static inline bool try_to_claim_workgroup(
> retry:
> if (grp->next == Z_EROFS_VLE_WORKGRP_NIL) {
> /* type 1, nil workgroup */
> - if (Z_EROFS_VLE_WORKGRP_NIL != cmpxchg(&grp->next,
> - Z_EROFS_VLE_WORKGRP_NIL, *owned_head))
> + if (cmpxchg(&grp->next, Z_EROFS_VLE_WORKGRP_NIL,
> + *owned_head) != Z_EROFS_VLE_WORKGRP_NIL)
> goto retry;
>
> *owned_head = grp;
> @@ -262,8 +262,8 @@ static inline bool try_to_claim_workgroup(
>* be careful that its submission itself is governed
>* by the original owned chain.
>*/
> - if (Z_EROFS_VLE_WORKGRP_TAIL != cmpxchg(&grp->next,
> - Z_EROFS_VLE_WORKGRP_TAIL, *owned_head))
> + if (cmpxchg(&grp->next, Z_EROFS_VLE_WORKGRP_TAIL,
> + *owned_head) != Z_EROFS_VLE_WORKGRP_TAIL)
> goto retry;
>
> *owned_head = Z_EROFS_VLE_WORKGRP_TAIL;
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/3] staging: erofs: unzip_vle.c: Replace comparison to NULL.
Hi Cristian, On 2018/11/13 4:43, Cristian Sicilia wrote: > Replace equal to NULL with logic unary operator, > and removing not equal to NULL comparison. > > Signed-off-by: Cristian Sicilia Indeed, I have several pending changes for this file... But these coding style fixes looks good to me. I will rebase them on your work... Reviewed-by: Gao Xiang Thanks, Gao Xiang ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/3] staging: erofs: unzip_vle.c: Replace comparison to NULL.
Hi Greg,
On 2018/11/13 7:46, Greg Kroah-Hartman wrote:
> On Tue, Nov 13, 2018 at 12:31:58AM +0100, Cristian Sicilia wrote:
>> On Mon, Nov 12, 2018 at 11:46 PM Greg Kroah-Hartman <
>> gre...@linuxfoundation.org> wrote:
>>
>>> On Mon, Nov 12, 2018 at 09:43:57PM +0100, Cristian Sicilia wrote:
>>>> Replace equal to NULL with logic unary operator,
>>>> and removing not equal to NULL comparison.
>>>>
>>>> Signed-off-by: Cristian Sicilia
>>>> ---
>>>> drivers/staging/erofs/unzip_vle.c | 86
>>> +++
>>>> 1 file changed, 43 insertions(+), 43 deletions(-)
>>>>
>>>> diff --git a/drivers/staging/erofs/unzip_vle.c
>>> b/drivers/staging/erofs/unzip_vle.c
>>>> index 79d3ba6..1ffeeaa 100644
>>>> --- a/drivers/staging/erofs/unzip_vle.c
>>>> +++ b/drivers/staging/erofs/unzip_vle.c
>>>> @@ -20,8 +20,8 @@ static struct kmem_cache *z_erofs_workgroup_cachep
>>> __read_mostly;
>>>>
>>>> void z_erofs_exit_zip_subsystem(void)
>>>> {
>>>> - BUG_ON(z_erofs_workqueue == NULL);
>>>> - BUG_ON(z_erofs_workgroup_cachep == NULL);
>>>> + BUG_ON(!z_erofs_workqueue);
>>>> + BUG_ON(!z_erofs_workgroup_cachep);
>>>
>>> Long-term, all of these BUG_ON need to be removed as they imply that the
>>> developer has no idea what went wrong and can not recover. For
>>> something like this, we "know" these will be fine and odds are they can
>>> just be removed entirely.
>>>
>>>
>> Right, I'm watching how replace the BUG_ON with WARN_ON and check who call
>> this functions
>
> No, why would WARN_ON be any better? Systems run with "panic on warn"
> enabled and this would cause the machine to reboot. Why are these
> things even being checked in the first place if they are impossible to
> hit?
>
> If they really are impossible, remove the check. If they are not, then
> properly handle the logic if they are true.
I will remove the above BUG_ON()s since it looks redundant.
>
> Linus said the other day something like "programmers who use BUG_ON()
> don't know what their code does", and I agree. They are a crutch that
> we need to fix up in the whole kernel, not just staging.
I agree the phrase "programmers who use BUG_ON() don't know what their code
does".
and some potential race I think it cannot happen in principle, but I also want
to check them
on runtime via beta users, that should be avoided case by case.
erofs has another CONFIG_EROFS_FS_DEBUG switch to make some on-disk
assertions aggressively in development/debug mode, if CONFIG_EROFS_FS_DEBUG is
on,
DBG_BUGON will be a BUG_ON; otherwise, it also has error handling paths to deal
with them properly.
I have no idea whether I'm doing the right thing or not... such switch can also
be found in f2fs called "F2FS_CHECK_FS".
config F2FS_CHECK_FS
bool "F2FS consistency checking feature"
depends on F2FS_FS
help
Enables BUG_ONs which check the filesystem consistency in runtime.
If you want to improve the performance, say N.
Could you kindly give me some suggestions? Thanks..
>
>>>> destroy_workqueue(z_erofs_workqueue);
>>>> kmem_cache_destroy(z_erofs_workgroup_cachep);
>>>> @@ -39,7 +39,7 @@ static inline int init_unzip_workqueue(void)
>>>> WQ_UNBOUND | WQ_HIGHPRI | WQ_CPU_INTENSIVE,
>>>> onlinecpus + onlinecpus / 4);
>>>>
>>>> - return z_erofs_workqueue != NULL ? 0 : -ENOMEM;
>>>> + return z_erofs_workqueue ? 0 : -ENOMEM;
>>>
>>> I hate ?: notation that is not in a function parameter, any way you can
>>> just change this to:
>>> if (z_erofs_workqueue)
>>> return 0;
>>> return -ENOMEM;
OK, I will avoid these unnecessary ?: notations.
>>>
>>>
>> I will replace the ?: too
>>
>>
>>> Christian, this isn't your fault at all, I'm not rejecting this patch,
>>> just providing hints on what else you can do here :)
>>>
>>
>>
>> but (if I well understand) I will send a different patch for both fix,
>> right?
>
> Yes, nothing wrong with this one that I could see. I'll let the erofs
> maintainers review it first before applying it in a few days to my tree
These patches look good to me, and I will avoid this BUG_ON case by case as I
promised to Al
before moving out the staging tree.
Thanks,
Gao Xiang
>
> thanks,
>
> greg k-h
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 02/10] staging: erofs: fix race when the managed cache is enabled
When the managed cache is enabled, the last reference count
of a workgroup must be used for its workstation.
Otherwise, it could lead to incorrect (un)freezes in
the reclaim path, and it would be harmful.
A typical race as follows:
Thread 1 (In the reclaim path) Thread 2
workgroup_freeze(grp, 1)refcnt = 1
...
workgroup_unfreeze(grp, 1) refcnt = 1
workgroup_get(grp) refcnt = 2 (x)
workgroup_put(grp) refcnt = 1 (x)
...unexpected behaviors
* grp is detached but still used, which violates cache-managed
freeze constraint.
Reviewed-by: Chao Yu
Signed-off-by: Gao Xiang
---
drivers/staging/erofs/internal.h | 1 +
drivers/staging/erofs/utils.c| 131 +++
2 files changed, 93 insertions(+), 39 deletions(-)
diff --git a/drivers/staging/erofs/internal.h b/drivers/staging/erofs/internal.h
index 57575c7f5635..89dbd0888e53 100644
--- a/drivers/staging/erofs/internal.h
+++ b/drivers/staging/erofs/internal.h
@@ -250,6 +250,7 @@ static inline bool erofs_workgroup_get(struct
erofs_workgroup *grp, int *ocnt)
}
#define __erofs_workgroup_get(grp) atomic_inc(&(grp)->refcount)
+#define __erofs_workgroup_put(grp) atomic_dec(&(grp)->refcount)
extern int erofs_workgroup_put(struct erofs_workgroup *grp);
diff --git a/drivers/staging/erofs/utils.c b/drivers/staging/erofs/utils.c
index ea8a962e5c95..90de8d9195b7 100644
--- a/drivers/staging/erofs/utils.c
+++ b/drivers/staging/erofs/utils.c
@@ -83,12 +83,21 @@ int erofs_register_workgroup(struct super_block *sb,
grp = xa_tag_pointer(grp, tag);
- err = radix_tree_insert(&sbi->workstn_tree,
- grp->index, grp);
+ /*
+* Bump up reference count before making this workgroup
+* visible to other users in order to avoid potential UAF
+* without serialized by erofs_workstn_lock.
+*/
+ __erofs_workgroup_get(grp);
- if (!err) {
- __erofs_workgroup_get(grp);
- }
+ err = radix_tree_insert(&sbi->workstn_tree,
+ grp->index, grp);
+ if (unlikely(err))
+ /*
+* it's safe to decrease since the workgroup isn't visible
+* and refcount >= 2 (cannot be freezed).
+*/
+ __erofs_workgroup_put(grp);
erofs_workstn_unlock(sbi);
radix_tree_preload_end();
@@ -97,19 +106,91 @@ int erofs_register_workgroup(struct super_block *sb,
extern void erofs_workgroup_free_rcu(struct erofs_workgroup *grp);
+static void __erofs_workgroup_free(struct erofs_workgroup *grp)
+{
+ atomic_long_dec(&erofs_global_shrink_cnt);
+ erofs_workgroup_free_rcu(grp);
+}
+
int erofs_workgroup_put(struct erofs_workgroup *grp)
{
int count = atomic_dec_return(&grp->refcount);
if (count == 1)
atomic_long_inc(&erofs_global_shrink_cnt);
- else if (!count) {
- atomic_long_dec(&erofs_global_shrink_cnt);
- erofs_workgroup_free_rcu(grp);
- }
+ else if (!count)
+ __erofs_workgroup_free(grp);
return count;
}
+#ifdef EROFS_FS_HAS_MANAGED_CACHE
+/* for cache-managed case, customized reclaim paths exist */
+static void erofs_workgroup_unfreeze_final(struct erofs_workgroup *grp)
+{
+ erofs_workgroup_unfreeze(grp, 0);
+ __erofs_workgroup_free(grp);
+}
+
+bool erofs_try_to_release_workgroup(struct erofs_sb_info *sbi,
+ struct erofs_workgroup *grp,
+ bool cleanup)
+{
+ /*
+* for managed cache enabled, the refcount of workgroups
+* themselves could be < 0 (freezed). So there is no guarantee
+* that all refcount > 0 if managed cache is enabled.
+*/
+ if (!erofs_workgroup_try_to_freeze(grp, 1))
+ return false;
+
+ /*
+* note that all cached pages should be unlinked
+* before delete it from the radix tree.
+* Otherwise some cached pages of an orphan old workgroup
+* could be still linked after the new one is available.
+*/
+ if (erofs_try_to_free_all_cached_pages(sbi, grp)) {
+ erofs_workgroup_unfreeze(grp, 1);
+ return false;
+ }
+
+ /* it is impossible to fail after we freeze the workgroup */
+ BUG_ON(xa_untag_pointer(radix_tree_delete(&sbi->workstn_tree,
+ grp->index)) != grp);
+
+ /*
+* if managed cache is enable, the last refcount
+* should indicate the related workstation.
+*/
+ erofs_workgroup_unfreeze_final(grp);
+ return true;
+}
+
+#else
+/* for nocache case
[PATCH 00/10] staging: erofs: decompression stability enhancement
Hi,
This patchset mainly focuses on erofs decompression stability, most of
them are found in the internal beta test. Some patches which are still
in testing or reviewing aren't included in this patchset and will be
sent after carefully testing.
Thanks,
Gao Xiang
Gao Xiang (10):
staging: erofs: fix `trace_erofs_readpage' position
staging: erofs: fix race when the managed cache is enabled
staging: erofs: atomic_cond_read_relaxed on ref-locked workgroup
staging: erofs: fix `erofs_workgroup_{try_to_freeze, unfreeze}'
staging: erofs: add a full barrier in erofs_workgroup_unfreeze
staging: erofs: fix the definition of DBG_BUGON
staging: erofs: separate into init_once / always
staging: erofs: locked before registering for all new workgroups
staging: erofs: decompress asynchronously if PG_readahead page at
first
staging: erofs: rename strange variable names in z_erofs_vle_frontend
drivers/staging/erofs/internal.h | 69
drivers/staging/erofs/unzip_vle.c | 78 ---
drivers/staging/erofs/utils.c | 131 ++
3 files changed, 190 insertions(+), 88 deletions(-)
--
2.14.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 01/10] staging: erofs: fix `trace_erofs_readpage' position
`trace_erofs_readpage' should be placed in .readpage()
rather than in the internal `z_erofs_do_read_page'.
Fixes: 284db12cfda3 ("staging: erofs: add trace points for reading zipped data")
Reviewed-by: Chen Gong
Reviewed-by: Chao Yu
Signed-off-by: Gao Xiang
---
drivers/staging/erofs/unzip_vle.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/erofs/unzip_vle.c
b/drivers/staging/erofs/unzip_vle.c
index 6a283f618f46..ede3383ac601 100644
--- a/drivers/staging/erofs/unzip_vle.c
+++ b/drivers/staging/erofs/unzip_vle.c
@@ -598,8 +598,6 @@ static int z_erofs_do_read_page(struct z_erofs_vle_frontend
*fe,
unsigned int cur, end, spiltted, index;
int err = 0;
- trace_erofs_readpage(page, false);
-
/* register locked file pages as online pages in pack */
z_erofs_onlinepage_init(page);
@@ -1288,6 +1286,8 @@ static int z_erofs_vle_normalaccess_readpage(struct file
*file,
int err;
LIST_HEAD(pagepool);
+ trace_erofs_readpage(page, false);
+
#if (EROFS_FS_ZIP_CACHE_LVL >= 2)
f.cachedzone_la = (erofs_off_t)page->index << PAGE_SHIFT;
#endif
--
2.14.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 03/10] staging: erofs: atomic_cond_read_relaxed on ref-locked workgroup
It's better to use atomic_cond_read_relaxed, which is implemented
in hardware instructions to monitor a variable changes currently
for ARM64, instead of open-coded busy waiting.
Reviewed-by: Chao Yu
Signed-off-by: Gao Xiang
---
drivers/staging/erofs/internal.h | 30 ++
1 file changed, 18 insertions(+), 12 deletions(-)
diff --git a/drivers/staging/erofs/internal.h b/drivers/staging/erofs/internal.h
index 89dbd0888e53..eb80ba44d072 100644
--- a/drivers/staging/erofs/internal.h
+++ b/drivers/staging/erofs/internal.h
@@ -221,23 +221,29 @@ static inline void erofs_workgroup_unfreeze(
preempt_enable();
}
+#if defined(CONFIG_SMP)
+static inline int erofs_wait_on_workgroup_freezed(struct erofs_workgroup *grp)
+{
+ return atomic_cond_read_relaxed(&grp->refcount,
+ VAL != EROFS_LOCKED_MAGIC);
+}
+#else
+static inline int erofs_wait_on_workgroup_freezed(struct erofs_workgroup *grp)
+{
+ int v = atomic_read(&grp->refcount);
+
+ /* workgroup is never freezed on uniprocessor systems */
+ DBG_BUGON(v == EROFS_LOCKED_MAGIC);
+ return v;
+}
+#endif
+
static inline bool erofs_workgroup_get(struct erofs_workgroup *grp, int *ocnt)
{
- const int locked = (int)EROFS_LOCKED_MAGIC;
int o;
repeat:
- o = atomic_read(&grp->refcount);
-
- /* spin if it is temporarily locked at the reclaim path */
- if (unlikely(o == locked)) {
-#if defined(CONFIG_SMP) || defined(CONFIG_DEBUG_SPINLOCK)
- do
- cpu_relax();
- while (atomic_read(&grp->refcount) == locked);
-#endif
- goto repeat;
- }
+ o = erofs_wait_on_workgroup_freezed(grp);
if (unlikely(o <= 0))
return -1;
--
2.14.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 05/10] staging: erofs: add a full barrier in erofs_workgroup_unfreeze
Just like other generic locks, insert a full barrier
in case of memory reorder.
Reviewed-by: Chao Yu
Signed-off-by: Gao Xiang
---
drivers/staging/erofs/internal.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/staging/erofs/internal.h b/drivers/staging/erofs/internal.h
index 2e0ef92c138b..f77653d33633 100644
--- a/drivers/staging/erofs/internal.h
+++ b/drivers/staging/erofs/internal.h
@@ -209,6 +209,7 @@ static inline bool erofs_workgroup_try_to_freeze(struct
erofs_workgroup *grp,
static inline void erofs_workgroup_unfreeze(struct erofs_workgroup *grp,
int orig_val)
{
+ smp_mb();
atomic_set(&grp->refcount, orig_val);
preempt_enable();
}
--
2.14.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 04/10] staging: erofs: fix `erofs_workgroup_{try_to_freeze, unfreeze}'
There are two minor issues in the current freeze interface:
1) Freeze interfaces have not related with CONFIG_DEBUG_SPINLOCK,
therefore fix the incorrect conditions;
2) For SMP platforms, it should also disable preemption before
doing atomic_cmpxchg in case that some high priority tasks
preempt between atomic_cmpxchg and disable_preempt, then spin
on the locked refcount later.
Reviewed-by: Chao Yu
Signed-off-by: Gao Xiang
---
drivers/staging/erofs/internal.h | 41
1 file changed, 25 insertions(+), 16 deletions(-)
diff --git a/drivers/staging/erofs/internal.h b/drivers/staging/erofs/internal.h
index eb80ba44d072..2e0ef92c138b 100644
--- a/drivers/staging/erofs/internal.h
+++ b/drivers/staging/erofs/internal.h
@@ -194,40 +194,49 @@ struct erofs_workgroup {
#define EROFS_LOCKED_MAGIC (INT_MIN | 0xE0F510CCL)
-static inline bool erofs_workgroup_try_to_freeze(
- struct erofs_workgroup *grp, int v)
+#if defined(CONFIG_SMP)
+static inline bool erofs_workgroup_try_to_freeze(struct erofs_workgroup *grp,
+int val)
{
-#if defined(CONFIG_SMP) || defined(CONFIG_DEBUG_SPINLOCK)
- if (v != atomic_cmpxchg(&grp->refcount,
- v, EROFS_LOCKED_MAGIC))
- return false;
preempt_disable();
-#else
- preempt_disable();
- if (atomic_read(&grp->refcount) != v) {
+ if (val != atomic_cmpxchg(&grp->refcount, val, EROFS_LOCKED_MAGIC)) {
preempt_enable();
return false;
}
-#endif
return true;
}
-static inline void erofs_workgroup_unfreeze(
- struct erofs_workgroup *grp, int v)
+static inline void erofs_workgroup_unfreeze(struct erofs_workgroup *grp,
+ int orig_val)
{
-#if defined(CONFIG_SMP) || defined(CONFIG_DEBUG_SPINLOCK)
- atomic_set(&grp->refcount, v);
-#endif
+ atomic_set(&grp->refcount, orig_val);
preempt_enable();
}
-#if defined(CONFIG_SMP)
static inline int erofs_wait_on_workgroup_freezed(struct erofs_workgroup *grp)
{
return atomic_cond_read_relaxed(&grp->refcount,
VAL != EROFS_LOCKED_MAGIC);
}
#else
+static inline bool erofs_workgroup_try_to_freeze(struct erofs_workgroup *grp,
+int val)
+{
+ preempt_disable();
+ /* no need to spin on UP platforms, let's just disable preemption. */
+ if (val != atomic_read(&grp->refcount)) {
+ preempt_enable();
+ return false;
+ }
+ return true;
+}
+
+static inline void erofs_workgroup_unfreeze(struct erofs_workgroup *grp,
+ int orig_val)
+{
+ preempt_enable();
+}
+
static inline int erofs_wait_on_workgroup_freezed(struct erofs_workgroup *grp)
{
int v = atomic_read(&grp->refcount);
--
2.14.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 06/10] staging: erofs: fix the definition of DBG_BUGON
It's better not to positively BUG_ON the kernel, however developers
need a way to locate issues as soon as possible.
DBG_BUGON is introduced and it could only crash when EROFS_FS_DEBUG
(EROFS developping feature) is on. It is helpful for developers
to find and solve bugs quickly.
Previously, DBG_BUGON is defined as ((void)0) if EROFS_FS_DEBUG is off,
but some unused variable warnings as follows could occur:
drivers/staging/erofs/unzip_vle.c: In function ‘init_always’:
drivers/staging/erofs/unzip_vle.c:61:33: warning: unused variable ‘work’
[-Wunused-variable]
struct z_erofs_vle_work *const work =
^~~~
Fix it to #define DBG_BUGON(x) ((void)(x)).
Reviewed-by: Chao Yu
Signed-off-by: Gao Xiang
---
drivers/staging/erofs/internal.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/erofs/internal.h b/drivers/staging/erofs/internal.h
index f77653d33633..0aa2a41b9885 100644
--- a/drivers/staging/erofs/internal.h
+++ b/drivers/staging/erofs/internal.h
@@ -39,7 +39,7 @@
#define debugln(x, ...) ((void)0)
#define dbg_might_sleep() ((void)0)
-#define DBG_BUGON(...) ((void)0)
+#define DBG_BUGON(x)((void)(x))
#endif
enum {
--
2.14.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 07/10] staging: erofs: separate into init_once / always
`z_erofs_vle_workgroup' is heavily generated in the decompression,
for example, it resets 32 bytes redundantly for 64-bit platforms
even through Z_EROFS_VLE_INLINE_PAGEVECS + Z_EROFS_CLUSTER_MAX_PAGES,
default 4, pages are stored in `z_erofs_vle_workgroup'.
As an another example, `struct mutex' takes 72 bytes for our kirin
64-bit platforms, it's unnecessary to be reseted at first and
be initialized each time.
Let's avoid filling all `z_erofs_vle_workgroup' with 0 at first
since most fields are reinitialized to meaningful values later,
and pagevec is no need to initialized at all.
Reviewed-by: Chao Yu
Signed-off-by: Gao Xiang
---
drivers/staging/erofs/unzip_vle.c | 34 +-
1 file changed, 29 insertions(+), 5 deletions(-)
diff --git a/drivers/staging/erofs/unzip_vle.c
b/drivers/staging/erofs/unzip_vle.c
index ede3383ac601..4e5843e8ee35 100644
--- a/drivers/staging/erofs/unzip_vle.c
+++ b/drivers/staging/erofs/unzip_vle.c
@@ -43,12 +43,38 @@ static inline int init_unzip_workqueue(void)
return z_erofs_workqueue ? 0 : -ENOMEM;
}
+static void init_once(void *ptr)
+{
+ struct z_erofs_vle_workgroup *grp = ptr;
+ struct z_erofs_vle_work *const work =
+ z_erofs_vle_grab_primary_work(grp);
+ unsigned int i;
+
+ mutex_init(&work->lock);
+ work->nr_pages = 0;
+ work->vcnt = 0;
+ for (i = 0; i < Z_EROFS_CLUSTER_MAX_PAGES; ++i)
+ grp->compressed_pages[i] = NULL;
+}
+
+static void init_always(struct z_erofs_vle_workgroup *grp)
+{
+ struct z_erofs_vle_work *const work =
+ z_erofs_vle_grab_primary_work(grp);
+
+ atomic_set(&grp->obj.refcount, 1);
+ grp->flags = 0;
+
+ DBG_BUGON(work->nr_pages);
+ DBG_BUGON(work->vcnt);
+}
+
int __init z_erofs_init_zip_subsystem(void)
{
z_erofs_workgroup_cachep =
kmem_cache_create("erofs_compress",
Z_EROFS_WORKGROUP_SIZE, 0,
- SLAB_RECLAIM_ACCOUNT, NULL);
+ SLAB_RECLAIM_ACCOUNT, init_once);
if (z_erofs_workgroup_cachep) {
if (!init_unzip_workqueue())
@@ -370,10 +396,11 @@ z_erofs_vle_work_register(const struct
z_erofs_vle_work_finder *f,
BUG_ON(grp);
/* no available workgroup, let's allocate one */
- grp = kmem_cache_zalloc(z_erofs_workgroup_cachep, GFP_NOFS);
+ grp = kmem_cache_alloc(z_erofs_workgroup_cachep, GFP_NOFS);
if (unlikely(!grp))
return ERR_PTR(-ENOMEM);
+ init_always(grp);
grp->obj.index = f->idx;
grp->llen = map->m_llen;
@@ -381,7 +408,6 @@ z_erofs_vle_work_register(const struct
z_erofs_vle_work_finder *f,
(map->m_flags & EROFS_MAP_ZIPPED) ?
Z_EROFS_VLE_WORKGRP_FMT_LZ4 :
Z_EROFS_VLE_WORKGRP_FMT_PLAIN);
- atomic_set(&grp->obj.refcount, 1);
/* new workgrps have been claimed as type 1 */
WRITE_ONCE(grp->next, *f->owned_head);
@@ -394,8 +420,6 @@ z_erofs_vle_work_register(const struct
z_erofs_vle_work_finder *f,
work = z_erofs_vle_grab_primary_work(grp);
work->pageofs = f->pageofs;
- mutex_init(&work->lock);
-
if (gnew) {
int err = erofs_register_workgroup(f->sb, &grp->obj, 0);
--
2.14.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 10/10] staging: erofs: rename strange variable names in z_erofs_vle_frontend
Previously, 2 members called `initial' and `cachedzone_la' are used
for applying caching policy (whether the workgroup is at either end),
which are hard to understand, rename them to `backmost' and `headoffset'.
Reviewed-by: Chao Yu
Signed-off-by: Gao Xiang
---
drivers/staging/erofs/unzip_vle.c | 25 +++--
1 file changed, 11 insertions(+), 14 deletions(-)
diff --git a/drivers/staging/erofs/unzip_vle.c
b/drivers/staging/erofs/unzip_vle.c
index 824d2c12c2f3..1ef178e7ac39 100644
--- a/drivers/staging/erofs/unzip_vle.c
+++ b/drivers/staging/erofs/unzip_vle.c
@@ -586,10 +586,9 @@ struct z_erofs_vle_frontend {
z_erofs_vle_owned_workgrp_t owned_head;
- bool initial;
-#if (EROFS_FS_ZIP_CACHE_LVL >= 2)
- erofs_off_t cachedzone_la;
-#endif
+ /* used for applying cache strategy on the fly */
+ bool backmost;
+ erofs_off_t headoffset;
};
#define VLE_FRONTEND_INIT(__i) { \
@@ -600,7 +599,7 @@ struct z_erofs_vle_frontend {
}, \
.builder = VLE_WORK_BUILDER_INIT(), \
.owned_head = Z_EROFS_VLE_WORKGRP_TAIL, \
- .initial = true, }
+ .backmost = true, }
static int z_erofs_do_read_page(struct z_erofs_vle_frontend *fe,
struct page *page,
@@ -643,7 +642,7 @@ static int z_erofs_do_read_page(struct z_erofs_vle_frontend
*fe,
debugln("%s: [out-of-range] pos %llu", __func__, offset + cur);
if (z_erofs_vle_work_iter_end(builder))
- fe->initial = false;
+ fe->backmost = false;
map->m_la = offset + cur;
map->m_llen = 0;
@@ -669,8 +668,8 @@ static int z_erofs_do_read_page(struct z_erofs_vle_frontend
*fe,
erofs_blknr(map->m_pa),
grp->compressed_pages, erofs_blknr(map->m_plen),
/* compressed page caching selection strategy */
- fe->initial | (EROFS_FS_ZIP_CACHE_LVL >= 2 ?
- map->m_la < fe->cachedzone_la : 0));
+ fe->backmost | (EROFS_FS_ZIP_CACHE_LVL >= 2 ?
+ map->m_la < fe->headoffset : 0));
if (noio_outoforder && builder_is_followed(builder))
builder->role = Z_EROFS_VLE_WORK_PRIMARY;
@@ -1316,9 +1315,8 @@ static int z_erofs_vle_normalaccess_readpage(struct file
*file,
trace_erofs_readpage(page, false);
-#if (EROFS_FS_ZIP_CACHE_LVL >= 2)
- f.cachedzone_la = (erofs_off_t)page->index << PAGE_SHIFT;
-#endif
+ f.headoffset = (erofs_off_t)page->index << PAGE_SHIFT;
+
err = z_erofs_do_read_page(&f, page, &pagepool);
(void)z_erofs_vle_work_iter_end(&f.builder);
@@ -1354,9 +1352,8 @@ static int z_erofs_vle_normalaccess_readpages(struct file
*filp,
trace_erofs_readpages(mapping->host, lru_to_page(pages),
nr_pages, false);
-#if (EROFS_FS_ZIP_CACHE_LVL >= 2)
- f.cachedzone_la = (erofs_off_t)lru_to_page(pages)->index << PAGE_SHIFT;
-#endif
+ f.headoffset = (erofs_off_t)lru_to_page(pages)->index << PAGE_SHIFT;
+
for (; nr_pages; --nr_pages) {
struct page *page = lru_to_page(pages);
--
2.14.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 08/10] staging: erofs: locked before registering for all new workgroups
Let's make sure that the one registering a workgroup will also
take the primary work lock at first for two reasons:
1) There's no need to introduce such a race window (and consequently
overhead) between registering and locking, other tasks could break
in by chance, and the race seems unnecessary (no benefit at all);
2) It's better to take the primary work when a workgroup
is registered to apply the cache managed policy, for example,
if some other tasks break in, it could turn into the in-place
decompression rather than use as the cached decompression.
Reviewed-by: Chao Yu
Signed-off-by: Gao Xiang
---
drivers/staging/erofs/unzip_vle.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/erofs/unzip_vle.c
b/drivers/staging/erofs/unzip_vle.c
index 4e5843e8ee35..a1376f3c6065 100644
--- a/drivers/staging/erofs/unzip_vle.c
+++ b/drivers/staging/erofs/unzip_vle.c
@@ -420,18 +420,22 @@ z_erofs_vle_work_register(const struct
z_erofs_vle_work_finder *f,
work = z_erofs_vle_grab_primary_work(grp);
work->pageofs = f->pageofs;
+ /* lock all primary followed works before visible to others */
+ if (unlikely(!mutex_trylock(&work->lock)))
+ /* for a new workgroup, try_lock *never* fails */
+ DBG_BUGON(1);
+
if (gnew) {
int err = erofs_register_workgroup(f->sb, &grp->obj, 0);
if (err) {
+ mutex_unlock(&work->lock);
kmem_cache_free(z_erofs_workgroup_cachep, grp);
return ERR_PTR(-EAGAIN);
}
}
*f->owned_head = *f->grp_ret = grp;
-
- mutex_lock(&work->lock);
return work;
}
--
2.14.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 09/10] staging: erofs: decompress asynchronously if PG_readahead page at first
For the case of nr_to_read == lookahead_size, it is better to
decompress asynchronously as well since no page will be needed immediately.
Reviewed-by: Chao Yu
Signed-off-by: Gao Xiang
---
drivers/staging/erofs/unzip_vle.c | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/erofs/unzip_vle.c
b/drivers/staging/erofs/unzip_vle.c
index a1376f3c6065..824d2c12c2f3 100644
--- a/drivers/staging/erofs/unzip_vle.c
+++ b/drivers/staging/erofs/unzip_vle.c
@@ -1344,8 +1344,8 @@ static int z_erofs_vle_normalaccess_readpages(struct file
*filp,
{
struct inode *const inode = mapping->host;
struct erofs_sb_info *const sbi = EROFS_I_SB(inode);
- const bool sync = __should_decompress_synchronously(sbi, nr_pages);
+ bool sync = __should_decompress_synchronously(sbi, nr_pages);
struct z_erofs_vle_frontend f = VLE_FRONTEND_INIT(inode);
gfp_t gfp = mapping_gfp_constraint(mapping, GFP_KERNEL);
struct page *head = NULL;
@@ -1363,6 +1363,13 @@ static int z_erofs_vle_normalaccess_readpages(struct
file *filp,
prefetchw(&page->flags);
list_del(&page->lru);
+ /*
+* A pure asynchronous readahead is indicated if
+* a PG_readahead marked page is hitted at first.
+* Let's also do asynchronous decompression for this case.
+*/
+ sync &= !(PageReadahead(page) && !head);
+
if (add_to_page_cache_lru(page, mapping, page->index, gfp)) {
list_add(&page->lru, &pagepool);
continue;
--
2.14.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 04/10] staging: erofs: fix `erofs_workgroup_{try_to_freeze, unfreeze}'
Hi Greg, On 2018/11/22 18:21, Greg Kroah-Hartman wrote: > On Tue, Nov 20, 2018 at 10:34:19PM +0800, Gao Xiang wrote: >> There are two minor issues in the current freeze interface: >> >>1) Freeze interfaces have not related with CONFIG_DEBUG_SPINLOCK, >> therefore fix the incorrect conditions; >> >>2) For SMP platforms, it should also disable preemption before >> doing atomic_cmpxchg in case that some high priority tasks >> preempt between atomic_cmpxchg and disable_preempt, then spin >> on the locked refcount later. > > spinning on a refcount implies that you are trying to do your own type > of locking. Why not use the in-kernel locking api instead? It will > always do better than trying to do your own logic as the developers > there know locking across all types of cpus better than filesystem > developers :) It is because refcount also plays a role as a spinlock on a specific value (== EROFS_LOCKED_MAGIC), no need to introduce such a value since the spin window is small. Thanks, Gao Xiang > > thanks, > > greg k-h > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 07/10] staging: erofs: separate into init_once / always
Hi Greg, On 2018/11/22 18:23, Greg Kroah-Hartman wrote: >> + >> +DBG_BUGON(work->nr_pages); >> +DBG_BUGON(work->vcnt); > How can these ever be triggered? I understand the need for debugging > code when you are writing code, but at this point it shouldn't be needed > anymore, right? I need to avoid some fields is not 0 when the new workgroup is created (because work->nr_pages and work->vcnt == 0 usually after the previous workgroup is freed). But that is not obvious, it is promised by the current logic. In order to not introduce such a issue in the future, or there are some potential race (work->nr_pages and work->vcnt != 0 when the previous workgroup == 0), it need to be noticed to developpers as early as possible. Thanks, Gao Xiang > > thanks, > > greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 08/10] staging: erofs: locked before registering for all new workgroups
Hi Greg, On 2018/11/22 18:24, Greg Kroah-Hartman wrote: >> +/* lock all primary followed works before visible to others */ >> +if (unlikely(!mutex_trylock(&work->lock))) >> +/* for a new workgroup, try_lock *never* fails */ >> +DBG_BUGON(1); > Again, drop this, if it never fails, then there's no need for this. If > it can fail, then properly handle it. > > And trylock can fail, so this needs to be fixed. OK, I will drop this. Thanks, Gao Xiang > > thanks, ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 02/10] staging: erofs: fix race when the managed cache is enabled
Hi Greg,
On 2018/11/22 18:17, Greg Kroah-Hartman wrote:
> Any specific reason why you are not using the refcount.h api instead of
> "doing it yourself" with atomic_inc/dec()?
>
> I'm not rejecting this, just curious.
As I explained in the previous email,
Re: [PATCH 04/10] staging: erofs: fix `erofs_workgroup_{try_to_freeze,
unfreeze}'
we need such a function when the value is >= 0, it plays as a refcount,
but when the refcount == EROFS_LOCKED_MAGIC (<0, but not 0 as refcount.h),
and actually there is no need to introduce a seperate spinlock_t because
we don't actually care about its performance (rarely locked). and
the corresponding struct is too large for now, we need to decrease its size.
Thanks,
Gao Xiang
>
> thanks,
>
> greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 01/10] staging: erofs: fix `trace_erofs_readpage' position
Hi Greg,
On 2018/11/22 18:19, Greg Kroah-Hartman wrote:
> On Tue, Nov 20, 2018 at 10:34:16PM +0800, Gao Xiang wrote:
>> `trace_erofs_readpage' should be placed in .readpage()
>> rather than in the internal `z_erofs_do_read_page'.
> Why? What happens with the code today?
trace_erofs_readpage is used to trace .readpage() interface (it represents sync
read)
hook rather than its internal implementation z_erofs_do_read_page (which both
.readpage()
and .readpages() uses it). Chen Gong places the tracepoint to a wrong place by
mistake.
And we found it by our internal test using this tracepoint.
>
>> Fixes: 284db12cfda3 ("staging: erofs: add trace points for reading zipped
>> data")
> Should this get into 4.20-final?
I think so, which is not very important but I think it should be fixed...
Thanks,
Gao Xiang
>
> thanks,
>
> greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 05/10] staging: erofs: add a full barrier in erofs_workgroup_unfreeze
Hi Greg, On 2018/11/22 18:22, Greg Kroah-Hartman wrote: > Please document this memory barrier. It does not make much sense to > me... Because we need to make the other observers noticing the latest values modified in this locking period before unfreezing the whole workgroup, one way is to use a memory barrier and the other way is to use ACQUIRE and RELEASE. we selected the first one. Hmmm...ok, I will add a simple message to explain this, but I think that is plain enough for a lock... Thanks, Gao Xiang > > thanks, > > greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 07/10] staging: erofs: separate into init_once / always
Hi Greg, On 2018/11/22 19:05, Greg Kroah-Hartman wrote: > On Thu, Nov 22, 2018 at 06:34:10PM +0800, Gao Xiang wrote: >> Hi Greg, >> >> On 2018/11/22 18:23, Greg Kroah-Hartman wrote: >>>> + >>>> + DBG_BUGON(work->nr_pages); >>>> + DBG_BUGON(work->vcnt); >>> How can these ever be triggered? I understand the need for debugging >>> code when you are writing code, but at this point it shouldn't be needed >>> anymore, right? >> >> I need to avoid some fields is not 0 when the new workgroup is created >> (because >> work->nr_pages and work->vcnt == 0 usually after the previous workgroup is >> freed). >> But that is not obvious, it is promised by the current logic. > > Then delete these lines if they can never happen :) I don't know how to observe such a race in our beta test and community users. Because if the kernel is crashed, we could collect the whole kernel dump to observe the memory and all registers, if we only have some warning, it will be not easy to get the state as early as possible. Thank, Gao Xiang > >> In order to not introduce such a issue in the future, or there are some >> potential >> race (work->nr_pages and work->vcnt != 0 when the previous workgroup == 0), >> it need >> to be noticed to developpers as early as possible. > > Then make it a real call, do not wrap it in odd macros that do not > really explain why it is "debugging only". Your code is "real" now, > make the logic real for all developers and users. > > thanks, > > greg k-h > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 04/10] staging: erofs: fix `erofs_workgroup_{try_to_freeze, unfreeze}'
Hi Greg, On 2018/11/22 19:05, Greg Kroah-Hartman wrote: > As I said in another email, doing two things with one variable is odd as > those are two different types of functions. I think lockref_put_or_lock do the similar thing, it is heavily used in dcache.c, but 1) 0 is special for us, we need lock it on a < 0 value rather than 0. 2) spinlock_t is too large for us because every compressed page will have the structure, but the locking rarely happens. Thanks, Gao Xiang > > greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 07/10] staging: erofs: separate into init_once / always
Hi Greg,
On 2018/11/22 19:26, Greg Kroah-Hartman wrote:
> On Thu, Nov 22, 2018 at 07:11:08PM +0800, Gao Xiang wrote:
>> Hi Greg,
>>
>> On 2018/11/22 19:05, Greg Kroah-Hartman wrote:
>>> On Thu, Nov 22, 2018 at 06:34:10PM +0800, Gao Xiang wrote:
>>>> Hi Greg,
>>>>
>>>> On 2018/11/22 18:23, Greg Kroah-Hartman wrote:
>>>>>> +
>>>>>> +DBG_BUGON(work->nr_pages);
>>>>>> +DBG_BUGON(work->vcnt);
>>>>> How can these ever be triggered? I understand the need for debugging
>>>>> code when you are writing code, but at this point it shouldn't be needed
>>>>> anymore, right?
>>>>
>>>> I need to avoid some fields is not 0 when the new workgroup is created
>>>> (because
>>>> work->nr_pages and work->vcnt == 0 usually after the previous workgroup is
>>>> freed).
>>>> But that is not obvious, it is promised by the current logic.
>>>
>>> Then delete these lines if they can never happen :)
>>
>> I don't know how to observe such a race in our beta test and community users.
>
> /*
>* Let developers know something went really wrong with their
>* initialization code
>*/
> if (!work->nr_pages) {
> pr_err("nr_pages == NULL!");
> WARN_ON(1);
> }
> if (!work->vcnt) {
> pr_err("vcnt == NULL!");
> WARN_ON(1);
> }
>
> or something like that.
>
> Don't make people rebuild your code with different options for
> debugging. That will never work in the 'real world' when people start
> using the code. You need to have things enabled for people all the
> time, which is why we have dynamic debugging in the kernel now, and not
> a zillion different "DRIVER_DEBUG" build options anymore.
>
>> Because if the kernel is crashed, we could collect the whole kernel dump to
>> observe the memory
>> and all registers, if we only have some warning, it will be not easy to get
>> the state as early as possible.
>
> When the kernel crashes, geting a dump is hard on almost all hardware.
> It is only rare systems that you can get a kernel dump.
This piece of code is already used in our hisilicon beta test, all memorydump,
registers, stack can be collected.
Apart from that, I observed many f2fs bugs are observed in this way and
many erofs bugs are collected in that way...sigh...
Thanks,
Gao Xiang
>
> thanks,
>
> greg k-h
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 02/10] staging: erofs: fix race when the managed cache is enabled
Hi Greg,
On 2018/11/22 19:06, Greg Kroah-Hartman wrote:
> On Thu, Nov 22, 2018 at 06:42:52PM +0800, Gao Xiang wrote:
>> Hi Greg,
>>
>> On 2018/11/22 18:17, Greg Kroah-Hartman wrote:
>>> Any specific reason why you are not using the refcount.h api instead of
>>> "doing it yourself" with atomic_inc/dec()?
>>>
>>> I'm not rejecting this, just curious.
>> As I explained in the previous email,
>> Re: [PATCH 04/10] staging: erofs: fix `erofs_workgroup_{try_to_freeze,
>> unfreeze}'
>>
>> we need such a function when the value is >= 0, it plays as a refcount,
>> but when the refcount == EROFS_LOCKED_MAGIC (<0, but not 0 as refcount.h),
>> and actually there is no need to introduce a seperate spinlock_t because
>> we don't actually care about its performance (rarely locked). and
>> the corresponding struct is too large for now, we need to decrease its size.
> Why do you need to decrease the size? How many of these structures are
> created?
As I said in the previous email, every compressed page will have a managed
structure
called erofs_workgroup, and it is heavily allocated like page/inode/dentry in
the erofs.
>
> And you will care about the performance when a lock is being held, as is
> evident by your logic to try to fix those issues in this patch series.
> Using a "real" lock will solve all of that and keep you from having to
> implement it all "by hand".
The function is much like lockref (aligned_u64 lock_count;) with the exception
as my previous email
explained.
Thanks,
Gao Xiang
>
> thanks,
>
> greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 07/10] staging: erofs: separate into init_once / always
Hi Greg, On 2018/11/22 19:26, Greg Kroah-Hartman wrote: > Don't make people rebuild your code with different options for > debugging. That will never work in the 'real world' when people start > using the code. You need to have things enabled for people all the > time, which is why we have dynamic debugging in the kernel now, and not > a zillion different "DRIVER_DEBUG" build options anymore. Actually, current erofs handle differently for beta users (in eng mode) and commercial users. CONFIG_EROFS_FS_DEBUG is enable for all beta users, after an observed expression is false, we could get the whole memorydump as early as possible. But for commercial users, there are no such observing points to promise the kernel stability and performance. It has helped us to find several bug, and I cannot find some alternative way to get the the first scene of the accident... Thanks, Gao Xiang ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
