Re: kernel BUG at fs/erofs/inode.c:LINE!

[Chao Yu]

Hi syzbot administrator,

CONFIG_EROFS_DEBUG was introduced for debug purpose during
development, this should not be enabled on release version.

Can you please turn off this config, and retest with erofs module?

Thanks,

On 2020/9/29 4:36, Gao Xiang wrote:

Hi,

On Mon, Sep 28, 2020 at 12:27:24AM -0700, syzbot wrote:

Hello,

syzbot found the following issue on:

HEAD commit:d1d2220c Add linux-next specific files for 20200924
git tree:   linux-next
console output: https://syzkaller.appspot.com/x/log.txt?x=166cb7d990
kernel config:  https://syzkaller.appspot.com/x/.config?x=254e028a642027c
dashboard link: https://syzkaller.appspot.com/bug?extid=8afc256dce324523609d
compiler:   gcc (GCC) 10.1.0-syz 20200507
syz repro:  https://syzkaller.appspot.com/x/repro.syz?x=127762c390
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=124ccf3b90

The issue was bisected to:

commit 382329a9d8553a98418a6f6e3425f0c288837897
Author: Gao Xiang 
Date:   Wed Aug 14 10:37:04 2019 +

 staging: erofs: differentiate unsupported on-disk format


if CONFIG_EROFS_DEBUG=y, the kernel will also BUG_ON on
currupted images in order to check potential mkfs fault.

So it's an expected behavior for now (if syzbot needs some more requirement,
e.g. differ from runtime error vs corrupted image error, I can do it if
needed.)

Thanks,
Gao Xiang



bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=149889e390
final oops: https://syzkaller.appspot.com/x/report.txt?x=169889e390
console output: https://syzkaller.appspot.com/x/log.txt?x=129889e390

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+8afc256dce3245236...@syzkaller.appspotmail.com
Fixes: 382329a9d855 ("staging: erofs: differentiate unsupported on-disk format")

erofs: (device loop0): erofs_read_inode: bogus i_mode (0) @ nid 36
[ cut here ]
kernel BUG at fs/erofs/inode.c:182!
invalid opcode:  [#1] PREEMPT SMP KASAN
CPU: 1 PID: 6895 Comm: syz-executor894 Not tainted 
5.9.0-rc6-next-20200924-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 
01/01/2011
RIP: 0010:erofs_read_inode fs/erofs/inode.c:182 [inline]
RIP: 0010:erofs_fill_inode fs/erofs/inode.c:235 [inline]
RIP: 0010:erofs_iget+0xfd8/0x2390 fs/erofs/inode.c:322
Code: 00 0f 85 aa 10 00 00 49 8b 7c 24 28 49 89 d8 44 89 e9 48 c7 c2 a0 9c ef 88 48 
c7 c6 40 9f ef 88 e8 b5 df b0 04 e8 88 5a 07 fe <0f> 0b e8 81 5a 07 fe 4c 89 e7 
4c 63 e3 e8 b6 61 5b fe e9 ed f0 ff
RSP: 0018:c90001017c10 EFLAGS: 00010293
RAX:  RBX: 0024 RCX: 
RDX: 8880a172e480 RSI: 836dd6e8 RDI: f52000202f72
RBP: 8880a8ca4480 R08: 0042 R09: 8880ae5319a7
R10:  R11:  R12: 8880854fd9b8
R13:  R14: ea0002a32900 R15: 
FS:  0108e880() GS:8880ae50() knlGS:
CS:  0010 DS:  ES:  CR0: 80050033
CR2: 0043eb80 CR3: a7edb000 CR4: 001506e0
DR0:  DR1:  DR2: 
DR3:  DR6: fffe0ff0 DR7: 0400
Call Trace:
  erofs_fc_fill_super+0xaa3/0x1010 fs/erofs/super.c:382
  get_tree_bdev+0x421/0x740 fs/super.c:1342
  vfs_get_tree+0x89/0x2f0 fs/super.c:1547
  do_new_mount fs/namespace.c:2896 [inline]
  path_mount+0x12ae/0x1e70 fs/namespace.c:3227
  __do_sys_mount fs/namespace.c:3438 [inline]
  __se_sys_mount fs/namespace.c:3411 [inline]
  __x64_sys_mount+0x278/0x2f0 fs/namespace.c:3411
  do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46
  entry_SYSCALL_64_after_hwframe+0x44/0xa9
RIP: 0033:0x446d6a
Code: b8 08 00 00 00 0f 05 48 3d 01 f0 ff ff 0f 83 fd ad fb ff c3 66 2e 0f 1f 84 00 
00 00 00 00 66 90 49 89 ca b8 a5 00 00 00 0f 05 <48> 3d 01 f0 ff ff 0f 83 da ad 
fb ff c3 66 0f 1f 84 00 00 00 00 00
RSP: 002b:7fffa8ef9ef8 EFLAGS: 0297 ORIG_RAX: 00a5
RAX: ffda RBX: 7fffa8ef9f50 RCX: 00446d6a
RDX: 2000 RSI: 2100 RDI: 7fffa8ef9f10
RBP: 7fffa8ef9f10 R08: 7fffa8ef9f50 R09: 7fff0015
R10:  R11: 0297 R12: 0001
R13: 0004 R14: 0003 R15: 0003
Modules linked in:
---[ end trace 66a5371a9bd8a3b2 ]---
RIP: 0010:erofs_read_inode fs/erofs/inode.c:182 [inline]
RIP: 0010:erofs_fill_inode fs/erofs/inode.c:235 [inline]
RIP: 0010:erofs_iget+0xfd8/0x2390 fs/erofs/inode.c:322
Code: 00 0f 85 aa 10 00 00 49 8b 7c 24 28 49 89 d8 44 89 e9 48 c7 c2 a0 9c ef 88 48 
c7 c6 40 9f ef 88 e8 b5 df b0 04 e8 88 5a 07 fe <0f> 0b e8 81 5a 07 fe 4c 89 e7 
4c 63 e3 e8 b6 61 5b fe e9 ed f0 ff
RSP: 0018:c90001017c10 EFLAGS: 00010293
RAX:  RBX: 0024 RCX: 
RDX: 8880a172e480 RSI: 836dd6e8 RDI: f52000202f72
RBP: 8880a8ca4480 R08: 

Re: [PATCH 10/10] errno.h: Provide EFSCORRUPTED for everybody

[Chao Yu]

On 2019/11/4 9:45, Valdis Kletnieks wrote:
> There's currently 6 filesystems that have the same #define. Move it
> into errno.h so it's defined in just one place.
> 
> Signed-off-by: Valdis Kletnieks 
> Acked-by: Darrick J. Wong 
> Reviewed-by: Jan Kara 
> Acked-by: Theodore Ts'o 

>  fs/erofs/internal.h  | 2 --

>  fs/f2fs/f2fs.h       | 1 -

Acked-by: Chao Yu 

Thanks,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 00/25] erofs: patchset addressing Christoph's comments

[Chao Yu]

On 2019/9/4 10:08, Gao Xiang wrote:
> Hi,
> 
> This patchset is based on the following patch by Pratik Shinde,
> https://lore.kernel.org/linux-erofs/20190830095615.10995-1-pratikshinde...@gmail.com/
> 
> All patches addressing Christoph's comments on v6, which are trivial,
> most deleted code are from erofs specific fault injection, which was
> followed f2fs and previously discussed in earlier topic [1], but
> let's follow what Christoph's said now.

Reviewed-by: Chao Yu 

Thanks,

> 
> Comments and suggestions are welcome...
> 
> [1] https://lore.kernel.org/r/1eed1e6b-f95e-aa8e-c3e7-e9870401e...@kernel.org/
> 
> changes since v1:
>  - leave some comments near the numbers to indicate where they are stored;
>  - avoid a u8 cast;
>  - use erofs_{err,info,dbg} and print sb->s_id as a prefix before
>the actual message;
>  - add a on-disk title in erofs_fs.h
>  - use feature_{compat,incompat} rather than features and requirements;
>  - suggestions on erofs_grab_bio:
>https://lore.kernel.org/r/20190902122016.gl15...@infradead.org/
>  - use compact/extended instead of erofs_inode_v1/v2 and
>i_format instead of i_advise;
>  - avoid chained if/else if/else if statements in erofs_read_inode;
>  - avoid erofs_vmap/vunmap wrappers;
>  - use read_cache_page_gfp for erofs_get_meta_page;
> 
> Gao Xiang (25):
>   erofs: remove all the byte offset comments
>   erofs: on-disk format should have explicitly assigned numbers
>   erofs: some macros are much more readable as a function
>   erofs: kill __packed for on-disk structures
>   erofs: update erofs_inode_is_data_compressed helper
>   erofs: use feature_incompat rather than requirements
>   erofs: better naming for erofs inode related stuffs
>   erofs: kill erofs_{init,exit}_inode_cache
>   erofs: use erofs_inode naming
>   erofs: update erofs_fs.h comments
>   erofs: update comments in inode.c
>   erofs: better erofs symlink stuffs
>   erofs: use dsb instead of layout for ondisk super_block
>   erofs: kill verbose debug info in erofs_fill_super
>   erofs: localize erofs_grab_bio()
>   erofs: kill prio and nofail of erofs_get_meta_page()
>   erofs: kill __submit_bio()
>   erofs: add "erofs_" prefix for common and short functions
>   erofs: kill all erofs specific fault injection
>   erofs: kill use_vmap module parameter
>   erofs: save one level of indentation
>   erofs: rename errln/infoln/debugln to erofs_{err,info,dbg}
>   erofs: use read_mapping_page instead of sb_bread
>   erofs: always use iget5_locked
>   erofs: use read_cache_page_gfp for erofs_get_meta_page
> 
>  Documentation/filesystems/erofs.txt |   9 -
>  fs/erofs/Kconfig|   7 -
>  fs/erofs/data.c | 118 +++
>  fs/erofs/decompressor.c |  76 +++
>  fs/erofs/dir.c  |  17 +-
>  fs/erofs/erofs_fs.h | 197 +-
>  fs/erofs/inode.c| 297 ++--
>  fs/erofs/internal.h | 192 --
>  fs/erofs/namei.c|  21 +-
>  fs/erofs/super.c| 282 +++---
>  fs/erofs/xattr.c|  41 ++--
>  fs/erofs/xattr.h|   4 +-
>  fs/erofs/zdata.c|  63 +++---
>  fs/erofs/zdata.h|   2 +-
>  fs/erofs/zmap.c |  73 +++
>  include/trace/events/erofs.h|  14 +-
>  16 files changed, 578 insertions(+), 835 deletions(-)
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v8 11/24] erofs: introduce xattr & posixacl support

[Chao Yu]

On 2019/9/2 22:20, David Sterba wrote:
> Oh right, I think the reasons are historical and that we can remove the
> options nowadays. From the compatibility POV this should be safe, with
> ACLs compiled out, no tool would use them, and no harm done when the
> code is present but not used.
> 
> There were some efforts by embedded guys to make parts of kernel more
> configurable to allow removing subsystems to reduce the final image
> size. In this case I don't think it would make any noticeable
> difference, eg. the size of fs/btrfs/acl.o on release config is 1.6KiB,
> while the whole module is over 1.3MiB.

Actually, btrfs's LOC is about 20 times larger than erofs's, acl part's LOC
could be very small one in btrfs.

EROFS can be slimmed about 10% size if we disable XATTR/ACL config, which is
worth to keep that, at least for now.

Thanks,


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v8 11/24] erofs: introduce xattr & posixacl support

[Chao Yu]

On 2019-9-2 21:06, David Sterba wrote:
> On Mon, Sep 02, 2019 at 05:57:11AM -0700, Christoph Hellwig wrote:
>>> +config EROFS_FS_XATTR
>>> +   bool "EROFS extended attributes"
>>> +   depends on EROFS_FS
>>> +   default y
>>> +   help
>>> + Extended attributes are name:value pairs associated with inodes by
>>> + the kernel or by users (see the attr(5) manual page, or visit
>>> +  for details).
>>> +
>>> + If unsure, say N.
>>> +
>>> +config EROFS_FS_POSIX_ACL
>>> +   bool "EROFS Access Control Lists"
>>> +   depends on EROFS_FS_XATTR
>>> +   select FS_POSIX_ACL
>>> +   default y
>>
>> Is there any good reason to make these optional these days?
> 
> I objected against adding so many config options, not to say for the
> standard features. The various cache strategies or other implementation
> details have been removed but I agree that making xattr/acl configurable
> is not necessary as well.

I can see similar *_ACL option in btrfs/ext4/xfs, should we remove them as well
due to the same reason?

Thanks,

> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Chao Yu]

On 2019/8/30 19:51, David Sterba wrote:
> On Fri, Aug 30, 2019 at 10:06:25AM +0800, Chao Yu wrote:
>> On 2019/8/29 23:43, Dan Carpenter wrote:
>>>> p.s. There are 2947 (un)likely places in fs/ directory.
>>>
>>> I was complaining about you adding new pointless ones, not existing
>>> ones.  The likely/unlikely annotations are supposed to be functional and
>>> not decorative.  I explained this very clearly.
>>>
>>> Probably most of the annotations in fs/ are wrong but they are also
>>> harmless except for the slight messiness.  However there are definitely
>>> some which are important so removing them all isn't a good idea.
>>
>> Hi Dan,
>>
>> Could you please pick up one positive example using likely and unlikely
>> correctly? so we can follow the example, rather than removing them all 
>> blindly.
> 
> Remove all of them and re-add with explanation if and how each is going
> to make things better. If you can't reason about, prove by benchmarks or
> point to inefficient asm code generated, then don't add them again.

It seems the result of it is strongly related to arch and compiler, if we readd
it after we just prove its gain only in one combination, I doubt we may suffer
regression in other combination in between archs and comilers. It looks like we
don't have any cheaper way to readd it? instead of verifying all/most 
combinations.

> 
> The feedback I got from CPU and compiler people over the years is not to
> bother using the annotations. CPUs are doing dynamic branch prediction
> and compilers are applying tons of optimizations.
> 
> GCC docs state about the builtin: "In general, you should prefer to use
> actual profile feedback for this (-fprofile-arcs), as programmers are
> notoriously bad at predicting how their programs actually perform."
> (https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html)

Yes, I agree with this. Thanks a lot for sharing your experience. :)

The removal change has done and been merged into Greg's tree, let's consider to
readd it later if possible as you suggested.

thanks,

> .
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v8 00/24] erofs: promote erofs from staging v8

[Chao Yu]

On 2019/8/15 12:41, Gao Xiang wrote:
> [I strip the previous cover letter, the old one can be found in v6:
>  https://lore.kernel.org/r/20190802125347.166018-1-gaoxian...@huawei.com/]
> 
> We'd like to submit a formal moving patch applied to staging tree
> for 5.4, before that we'd like to hear if there are some ACKs,
> suggestions or NAKs, objections of EROFS. Therefore, we can improve
> it in this round or rethink about the whole thing.
> 
> As related materials mentioned [1] [2], the goal of EROFS is to
> save extra storage space with guaranteed end-to-end performance
> for read-only files, which has better performance over exist Linux
> compression filesystems based on fixed-sized output compression
> and inplace decompression. It even has better performance in
> a large compression ratio range compared with generic uncompressed
> filesystems with proper CPU-storage combinations. And we think this
> direction is correct and a dedicated kernel team is continuously /
> actively working on improving it, enough testers and beta / end
> users using it.
> 
> EROFS has been applied to almost all in-service HUAWEI smartphones
> (Yes, the number is still increasing by time) and it seems like
> a success. It can be used in more wider scenarios. We think it's
> useful for Linux / Android OS community and it's the time moving
> out of staging.
> 
> In order to get started, latest stable mkfs.erofs is available at
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/xiang/erofs-utils.git -b dev
> 
> with README in the repository.
> 
> We are still tuning sequential read performance for ultra-fast
> speed NVME SSDs like Samsung 970PRO, but at least now you can
> try on your PC with some data with proper compression ratio,
> the latest Linux kernel, USB stick for convenience sake and
> a not very old-fashioned CPU. There are also benchmarks available
> in the above materials mentioned.
> 
> EROFS is a self-contained filesystem driver. Although there are
> still some TODOs to be more generic, we will actively keep on
> developping / tuning EROFS with the evolution of Linux kernel
> as the other in-kernel filesystems.
> 
> As I mentioned before in LSF/MM 2019, in the future, we'd like
> to generalize the decompression engine into a library for other
> fses to use after the whole system is mature like fscrypt.
> However, such metadata should be designed respectively for
> each fs, and synchronous metadata read cost will be larger
> than EROFS because of those ondisk limitation. Therefore EROFS
> is still a better choice for read-only scenarios.
> 
> EROFS is now ready for reviewing and moving, and the code is
> already cleaned up as shiny floors... Please kindly take some
> precious time, share your comments about EROFS and let us know
> your opinion about this. It's really important for us since
> generally speaking, we like to use Linux _in-tree_ stuffs rather
> than lack of supported out-of-tree / orphan stuffs as well.

EROFS proposes its very unique fixed-sized output compression and inplace
decompression framework joining into the ecosystem of compression filesystem, I
think it will enrich diversity of compression filesystem, and bring healthy
competition there.

I do believe this is the right time to promote erofs to fs/ directory, let it be
the formal member of filesystem clubhouse.

Acked-by: Chao Yu 

Thanks

> 
> Thank you in advance,
> Gao Xiang
> 
> [1] 
> https://kccncosschn19eng.sched.com/event/Nru2/erofs-an-introduction-and-our-smartphone-practice-xiang-gao-huawei
> [2] https://www.usenix.org/conference/atc19/presentation/gao
> 
> Changelog from v7:
>  o keep up with the latest staging tree in addition to
>the latest staging patch:
>https://lore.kernel.org/r/20190814103705.60698-1-gaoxian...@huawei.com/
>- use EUCLEAN for fs corruption cases suggested by Pavel;
>- turn EIO into EOPNOTSUPP for unsupported on-disk format;
>- fix all misused ENOTSUPP into EOPNOTSUPP pointed out by Chao;
>  o update cover letter
> 
> It can also be found in git at tag "erofs_2019-08-15" (will be shown later) 
> at:
>  https://git.kernel.org/pub/scm/linux/kernel/git/xiang/linux.git/
> 
> and the latest fs code is available at:
>  
> https://git.kernel.org/pub/scm/linux/kernel/git/xiang/linux.git/tree/fs/erofs?h=erofs-outofstaging
> 
> Changelog from v6:
>  o keep up with the latest staging patchset
>
> https://lore.kernel.org/linux-fsdevel/20190813023054.73126-1-gaoxian...@huawei.com/
>in order to fix the following cases:
>- inline erofs_inode_is_data_compressed() in erofs_fs.h;
>- remove incomplete cleancache;
>- remove all BUG_ON in EROFS.
>  o Removing the file names from the comments at the top of t

Re: [PATCH v3 6/7] erofs: remove all likely/unlikely annotations

[Chao Yu]

Xiang, the code itself looks clean to me.

Reviewed-by: Chao Yu 

Thanks,

On 2019/8/30 14:31, Gao Xiang wrote:
> Hi Chao,
> 
> On Fri, Aug 30, 2019 at 02:25:13PM +0800, Chao Yu wrote:
>> On 2019/8/30 11:36, Gao Xiang wrote:
>>> As Dan Carpenter suggested [1], I have to remove
>>> all erofs likely/unlikely annotations.
>>>
>>> [1] https://lore.kernel.org/linux-fsdevel/20190829154346.GK23584@kadam/
>>> Reported-by: Dan Carpenter 
>>> Signed-off-by: Gao Xiang 
>>
>> I suggest we can modify this by following good example rather than removing 
>> them
>> all, at least, the fuzzed random fields of disk layout handling should be 
>> very
>> rare case, I guess it's fine to use unlikely.
>>
>> To Dan, thoughts?
> 
> I think let's stop talking this anymore, I think we can do such
> a commit now and I think "folks have their own thoughts on it".
> 
> Could you kindly review and check this one? I did it by 'sed', could
> you double check it?
> 
> Thanks,
> Gao Xiang
> 
>>
>> Thanks,
>>
>>> ---
>>> no change.
>>>
>>>  fs/erofs/data.c | 22 ++---
>>>  fs/erofs/decompressor.c |  2 +-
>>>  fs/erofs/dir.c  | 14 ++---
>>>  fs/erofs/inode.c| 10 +-
>>>  fs/erofs/internal.h |  4 ++--
>>>  fs/erofs/namei.c| 14 ++---
>>>  fs/erofs/super.c| 16 +++
>>>  fs/erofs/utils.c| 12 +--
>>>  fs/erofs/xattr.c| 12 +--
>>>  fs/erofs/zdata.c| 44 -
>>>  fs/erofs/zmap.c |  8 
>>>  fs/erofs/zpvec.h|  6 +++---
>>>  12 files changed, 82 insertions(+), 82 deletions(-)
>>>
>>> diff --git a/fs/erofs/data.c b/fs/erofs/data.c
>>> index fda16ec8863e..0f2f1a839372 100644
>>> --- a/fs/erofs/data.c
>>> +++ b/fs/erofs/data.c
>>> @@ -27,7 +27,7 @@ static inline void read_endio(struct bio *bio)
>>> /* page is already locked */
>>> DBG_BUGON(PageUptodate(page));
>>>  
>>> -   if (unlikely(err))
>>> +   if (err)
>>> SetPageError(page);
>>> else
>>> SetPageUptodate(page);
>>> @@ -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)) {
>>> +   if (!page) {
>>> DBG_BUGON(nofail);
>>> return ERR_PTR(-ENOMEM);
>>> }
>>> @@ -70,7 +70,7 @@ struct page *__erofs_get_meta_page(struct super_block *sb,
>>> }
>>>  
>>> err = bio_add_page(bio, page, PAGE_SIZE, 0);
>>> -   if (unlikely(err != PAGE_SIZE)) {
>>> +   if (err != PAGE_SIZE) {
>>> err = -EFAULT;
>>> goto err_out;
>>> }
>>> @@ -81,7 +81,7 @@ struct page *__erofs_get_meta_page(struct super_block *sb,
>>> lock_page(page);
>>>  
>>> /* this page has been truncated by others */
>>> -   if (unlikely(page->mapping != mapping)) {
>>> +   if (page->mapping != mapping) {
>>>  unlock_repeat:
>>> unlock_page(page);
>>> put_page(page);
>>> @@ -89,7 +89,7 @@ struct page *__erofs_get_meta_page(struct super_block *sb,
>>> }
>>>  
>>> /* more likely a read error */
>>> -   if (unlikely(!PageUptodate(page))) {
>>> +   if (!PageUptodate(page)) {
>>> if (io_retries) {
>>> --io_retries;
>>> goto unlock_repeat;
>>> @@ -120,7 +120,7 @@ static int erofs_map_blocks_flatmode(struct inode 
>>> *inode,
>>> nblocks = DIV_ROUND_UP(inode->i_size, PAGE_SIZE);
>>> lastblk = nblocks - is_inode_flat_inline(inode);
>>>  
>>> -   if (unlikely(offset >= inode->i_size)) {
>>> +   if (offset >= inode->i_size) {
>>> /* leave out-of-bound access unmapped */
>>> map->m_flags = 0;
>>> map->m_plen = 0;
>>> @@ -170,7 +170,7 @@ static int erofs_map_blocks_flatmode(struct inode 
>>> *inode,
>>>  int ero

Re: [PATCH v3 7/7] erofs: redundant assignment in __erofs_get_meta_page()

[Chao Yu]

On 2019/8/30 11:36, Gao Xiang wrote:
> As Joe Perches suggested [1],
>   err = bio_add_page(bio, page, PAGE_SIZE, 0);
> - if (unlikely(err != PAGE_SIZE)) {
> + if (err != PAGE_SIZE) {
>   err = -EFAULT;
>   goto err_out;
>   }
> 
> The initial assignment to err is odd as it's not
> actually an error value -E but a int size
> from a unsigned int len.
> 
> Here the return is either 0 or PAGE_SIZE.
> 
> This would be more legible to me as:
> 
>   if (bio_add_page(bio, page, PAGE_SIZE, 0) != PAGE_SIZE) {
>   err = -EFAULT;
>   goto err_out;
>   }
> 
> [1] 
> https://lore.kernel.org/r/74c4784319b40deabfbaea92468f7e3ef44f1c96.ca...@perches.com/
> Reported-by: Joe Perches 
> Signed-off-by: Gao Xiang 

Reviewed-by: Chao Yu 

Thanks,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 6/7] erofs: remove all likely/unlikely annotations

[Chao Yu]

On 2019/8/30 11:36, Gao Xiang wrote:
> As Dan Carpenter suggested [1], I have to remove
> all erofs likely/unlikely annotations.
> 
> [1] https://lore.kernel.org/linux-fsdevel/20190829154346.GK23584@kadam/
> Reported-by: Dan Carpenter 
> Signed-off-by: Gao Xiang 

I suggest we can modify this by following good example rather than removing them
all, at least, the fuzzed random fields of disk layout handling should be very
rare case, I guess it's fine to use unlikely.

To Dan, thoughts?

Thanks,

> ---
> no change.
> 
>  fs/erofs/data.c | 22 ++---
>  fs/erofs/decompressor.c |  2 +-
>  fs/erofs/dir.c  | 14 ++---
>  fs/erofs/inode.c| 10 +-
>  fs/erofs/internal.h |  4 ++--
>  fs/erofs/namei.c| 14 ++---
>  fs/erofs/super.c| 16 +++
>  fs/erofs/utils.c| 12 +--
>  fs/erofs/xattr.c| 12 +--
>  fs/erofs/zdata.c| 44 -
>  fs/erofs/zmap.c |  8 
>  fs/erofs/zpvec.h|  6 +++---
>  12 files changed, 82 insertions(+), 82 deletions(-)
> 
> diff --git a/fs/erofs/data.c b/fs/erofs/data.c
> index fda16ec8863e..0f2f1a839372 100644
> --- a/fs/erofs/data.c
> +++ b/fs/erofs/data.c
> @@ -27,7 +27,7 @@ static inline void read_endio(struct bio *bio)
>   /* page is already locked */
>   DBG_BUGON(PageUptodate(page));
>  
> - if (unlikely(err))
> + if (err)
>   SetPageError(page);
>   else
>   SetPageUptodate(page);
> @@ -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)) {
> + if (!page) {
>   DBG_BUGON(nofail);
>   return ERR_PTR(-ENOMEM);
>   }
> @@ -70,7 +70,7 @@ struct page *__erofs_get_meta_page(struct super_block *sb,
>   }
>  
>   err = bio_add_page(bio, page, PAGE_SIZE, 0);
> - if (unlikely(err != PAGE_SIZE)) {
> + if (err != PAGE_SIZE) {
>   err = -EFAULT;
>   goto err_out;
>   }
> @@ -81,7 +81,7 @@ struct page *__erofs_get_meta_page(struct super_block *sb,
>   lock_page(page);
>  
>   /* this page has been truncated by others */
> - if (unlikely(page->mapping != mapping)) {
> + if (page->mapping != mapping) {
>  unlock_repeat:
>   unlock_page(page);
>   put_page(page);
> @@ -89,7 +89,7 @@ struct page *__erofs_get_meta_page(struct super_block *sb,
>   }
>  
>   /* more likely a read error */
> - if (unlikely(!PageUptodate(page))) {
> + if (!PageUptodate(page)) {
>   if (io_retries) {
>   --io_retries;
>   goto unlock_repeat;
> @@ -120,7 +120,7 @@ static int erofs_map_blocks_flatmode(struct inode *inode,
>   nblocks = DIV_ROUND_UP(inode->i_size, PAGE_SIZE);
>   lastblk = nblocks - is_inode_flat_inline(inode);
>  
> - if (unlikely(offset >= inode->i_size)) {
> + if (offset >= inode->i_size) {
>   /* leave out-of-bound access unmapped */
>   map->m_flags = 0;
>   map->m_plen = 0;
> @@ -170,7 +170,7 @@ static int erofs_map_blocks_flatmode(struct inode *inode,
>  int erofs_map_blocks(struct inode *inode,
>struct erofs_map_blocks *map, int flags)
>  {
> - if (unlikely(is_inode_layout_compression(inode))) {
> + if (is_inode_layout_compression(inode)) {
>   int err = z_erofs_map_blocks_iter(inode, map, flags);
>  
>   if (map->mpage) {
> @@ -218,11 +218,11 @@ static inline struct bio *erofs_read_raw_page(struct 
> bio *bio,
>   unsigned int blkoff;
>  
>   err = erofs_map_blocks(inode, , EROFS_GET_BLOCKS_RAW);
> - if (unlikely(err))
> + if (err)
>   goto err_out;
>  
>   /* zero out the holed page */
> - if (unlikely(!(map.m_flags & EROFS_MAP_MAPPED))) {
> + if (!(map.m_flags & EROFS_MAP_MAPPED)) {
>   zero_user_segment(page, 0, PAGE_SIZE);
>   SetPageUptodate(page);
>  
> @@ -315,7 +315,7 @@ static inline struct bio *erofs_read_raw_page(struct bio 
> *bio,
>  submit_bio_out:
>   __submit_bio(bio, REQ_OP_READ, 0);
>  
> - return unlikely(err) ? ERR_PTR(err) : NULL;
> + return err ? ERR_PTR(err) : NULL;
>  }
>  
>  /*
> @@ -377,7 +377,7 @@ static int erofs_raw_access_readpages(struct file *filp,
>   DBG_BUGON(!list_empty(pages));
>  
>   /* the rare case (end in gaps) */
> - if (unlikely(bio))
> + if (bio)
>   __submit_bio(bio, REQ_OP_READ, 0);
>   return 0;
>  }
> diff --git 

Re: [PATCH v3 5/7] erofs: kill erofs_{init,exit}_inode_cache

[Chao Yu]

On 2019/8/30 11:36, Gao Xiang wrote:
> As Christoph said [1] "having this function
> seems entirely pointless", I have to kill those.
> 
> filesystem  function name
> ext2,f2fs,ext4,isofs,squashfs,cifs,...  init_inodecache
> 
> In addition, add a "rcu_barrier()" when exit_fs();
> 
> [1] https://lore.kernel.org/r/20190829101545.gc20...@infradead.org/
> Reported-by: Christoph Hellwig 
> Signed-off-by: Gao Xiang 

Reviewed-by: Chao Yu 

Thanks,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 4/7] erofs: kill __packed for on-disk structures

[Chao Yu]

On 2019/8/30 11:36, Gao Xiang wrote:
> As Christoph claimed "Please don't add __packed" [1],
> I have to remove all __packed except struct erofs_dirent here.
> 
> Note that all on-disk fields except struct erofs_dirent
> (12 bytes with a 8-byte nid) in EROFS are naturally aligned.
> 
> [1] https://lore.kernel.org/lkml/20190829095954.gb20...@infradead.org/
> Reported-by: Christoph Hellwig 
> Signed-off-by: Gao Xiang 

Reviewed-by: Chao Yu 

Thanks,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 2/7] erofs: some macros are much more readable as a function

[Chao Yu]

On 2019/8/30 11:36, Gao Xiang wrote:
> As Christoph suggested [1], these marcos are much
> more readable as a function.
> 
> [1] https://lore.kernel.org/r/20190829095954.gb20...@infradead.org/
> Reported-by: Christoph Hellwig 
> Signed-off-by: Gao Xiang 

Reviewed-by: Chao Yu 

Thanks,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 3/7] erofs: use a better form for complicated on-disk fields

[Chao Yu]

On 2019/8/30 11:00, Gao Xiang wrote:
> As Joe Perches [1] suggested, let's use a better
> form to describe complicated on-disk fields.
> 
> p.s. it has different tab alignment looking between
>  the real file and patch file.
> p.p.s. due to changing a different form, some lines
>have to exceed 80 characters.
> [1] 
> https://lore.kernel.org/r/67d6efbbc9ac6db23215660cb970b7ef29dc0c1d.ca...@perches.com/
> Reported-by: Joe Perches 
> Signed-off-by: Gao Xiang 

Reviewed-by: Chao Yu 

Thanks,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 1/7] erofs: on-disk format should have explicitly assigned numbers

[Chao Yu]

On 2019/8/30 11:00, Gao Xiang wrote:
> As Christoph claimed [1], on-disk format should have
> explicitly assigned numbers. I have to change it.
> 
> [1] https://lore.kernel.org/r/20190829095954.gb20...@infradead.org/
> Reported-by: Christoph Hellwig 
> Signed-off-by: Gao Xiang 

Reviewed-by: Chao Yu 

Thanks,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: exfat: add exfat filesystem code to staging

[Chao Yu]

On 2019/8/29 23:43, Dan Carpenter wrote:
>> p.s. There are 2947 (un)likely places in fs/ directory.
> 
> I was complaining about you adding new pointless ones, not existing
> ones.  The likely/unlikely annotations are supposed to be functional and
> not decorative.  I explained this very clearly.
> 
> Probably most of the annotations in fs/ are wrong but they are also
> harmless except for the slight messiness.  However there are definitely
> some which are important so removing them all isn't a good idea.

Hi Dan,

Could you please pick up one positive example using likely and unlikely
correctly? so we can follow the example, rather than removing them all blindly.

Thanks,

> 
>> If you like, I will delete them all.
> 
> But for erofs, I don't think that any of the likely/unlikely calls have
> been thought about so I'm fine with removing all of them in one go.
> 
> regards,
> dan carpenter
> 
> .
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH RESEND] erofs: fix compile warnings when moving out include/trace/events/erofs.h

[Chao Yu]

On 2019-8-26 21:26, Gao Xiang wrote:
> As Stephon reported [1], many compile warnings are raised when
> moving out include/trace/events/erofs.h:
> 
> In file included from include/trace/events/erofs.h:8,
>  from :
> include/trace/events/erofs.h:28:37: warning: 'struct dentry' declared inside 
> parameter list will not be visible outside of this definition or declaration
>   TP_PROTO(struct inode *dir, struct dentry *dentry, unsigned int flags),
>  ^~
> include/linux/tracepoint.h:233:34: note: in definition of macro 
> '__DECLARE_TRACE'
>   static inline void trace_##name(proto)\
>   ^
> include/linux/tracepoint.h:396:24: note: in expansion of macro 'PARAMS'
>   __DECLARE_TRACE(name, PARAMS(proto), PARAMS(args),  \
> ^~
> include/linux/tracepoint.h:532:2: note: in expansion of macro 'DECLARE_TRACE'
>   DECLARE_TRACE(name, PARAMS(proto), PARAMS(args))
>   ^
> include/linux/tracepoint.h:532:22: note: in expansion of macro 'PARAMS'
>   DECLARE_TRACE(name, PARAMS(proto), PARAMS(args))
>   ^~
> include/trace/events/erofs.h:26:1: note: in expansion of macro 'TRACE_EVENT'
>  TRACE_EVENT(erofs_lookup,
>  ^~~
> include/trace/events/erofs.h:28:2: note: in expansion of macro 'TP_PROTO'
>   TP_PROTO(struct inode *dir, struct dentry *dentry, unsigned int flags),
>   ^~~~
> 
> That makes me very confused since most original EROFS tracepoint code
> was taken from f2fs, and finally I found
> 
> commit 43c78d88036e ("kbuild: compile-test kernel headers to ensure they are 
> self-contained")
> 
> It seems these warnings are generated from KERNEL_HEADER_TEST feature and
> ext4/f2fs tracepoint files were in blacklist.

For f2fs.h, it will be only used by f2fs module, I guess it's okay to let it
stay in blacklist...

> 
> Anyway, let's fix these issues for KERNEL_HEADER_TEST feature instead
> of adding to blacklist...
> 
> [1] https://lore.kernel.org/lkml/20190826162432.11100...@canb.auug.org.au/
> Reported-by: Stephen Rothwell 
> Signed-off-by: Gao Xiang 

Reviewed-by: Chao Yu 

Thanks,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 5/6] staging: erofs: detect potential multiref due to corrupted images

[Chao Yu]

On 2019-8-21 22:01, Gao Xiang wrote:
> As reported by erofs-utils fuzzer, currently, multiref
> (ondisk deduplication) hasn't been supported for now,
> we should forbid it properly.
> 
> Fixes: 3883a79abd02 ("staging: erofs: introduce VLE decompression support")
> Cc:  # 4.19+
> Signed-off-by: Gao Xiang 

Reviewed-by: Chao Yu 

Thanks,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] erofs: move erofs out of staging

[Chao Yu]

On 2019/8/20 16:46, Qu Wenruo wrote:
> [...]
>>
>> Yeah, it looks like we need searching more levels mapping to find the final
>> physical block address of inode/node/data in btrfs.
>>
>> IMO, in a little lazy way, we can reform and reuse existed function in
>> btrfs-progs which can find the mapping info of inode/node/data according to
>> specified ino or ino+pg_no.
> 
> Maybe no need to go as deep as ino.
> 
> What about just go physical bytenr? E.g. for XFS/EXT* choose a random
> bytenr. Then verify if that block is used, if not, try again.
> 
> If used, check if it's metadata. If not, try again.
> (feel free to corrupt data, in fact btrfs uses some data as space cache,
> so it should make some sense)
> 
> If metadata, corrupt that bytenr/bytenr range in the metadata block,
> regenerate checksum, call it a day and let kernel suffer.
> 
> For btrfs, just do extra physical -> logical convert in the first place,
> then follow the same workflow.
> It should work for any fs as long as it's on single device.

Agree, it will be easier to trigger random injection in specific area, and also
I agreed with Ted, some of the time we prefer to do injection in specified field
of meta, it looks developer needs to do more work for that.

> 
>>
>>>
>>> It may depends on the granularity. But definitely a good idea to do so
>>> in a generic way.
>>> Currently we depend on super kind student developers/reporters on such
>>
>> Yup, I just guess Wen Xu may be interested in working on a generic way to 
>> fuzz
>> filesystem, as I know they dig deep in filesystem code when doing fuzz.
> 
> Don't forget Yoon Jungyeon, I see more than one times he reported fuzzed
> images with proper reproducer and bugzilla links.

Of course I remember him. :)

I guess btrfs/f2fs should has improved their stability/robustness a lot due to
Jungyeon and Wen Xu's gret fuzz bug report.

> Even using his personal mail address, not school mail address.
> 
> Those guys are really awesome!
> 
>> BTW,
>> which impresses me is, constructing checkpoint by injecting one byte, and 
>> then
>> write a correct recalculated checksum value on that checkpoint, making that
>> checkpoint looks valid...
> 
> IIRC F2FS guys may be also investigating a similar mechanism, as they
> also got a hard fight against reports from those awesome reporters.

Actually, f2fs only support realtime fault injection framework, which allows us
to inject memory exhausting, IO error, lack of free blocks, shutdown... error
during fsstress test.

I do think f2fs needs that kind of tool later.

Thanks,

> 
> So such fuzzed image is a new trend for fs development.
> 
> Thanks,
> Qu
> 
>>
>> Thanks,
>>
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] erofs: move erofs out of staging

[Chao Yu]

On 2019/8/21 9:48, Darrick J. Wong wrote:
> On Wed, Aug 21, 2019 at 09:34:02AM +0800, Chao Yu wrote:
>> On 2019/8/20 23:56, Theodore Y. Ts'o wrote:
>>> The reason why there needs to be at least some file system specific
>>> code for fuzz testing is because for efficiency's sake, you don't want
>>> to fuzz every single bit in the file system, but just the ones which
>>> are most interesting (e.g., the metadata blocks).  For file systems
>>> which use checksum to protect against accidental corruption, the file
>>> system fuzzer needs to also fix up the checksums (since you can be
>>> sure malicious attackers will do this).
>>
>> Yup, IMO, if we really want such tool, it needs to:
>> - move all generic fuzz codes (trigger random fuzzing in meta/data area) into
>> that tool, and
>> - make filesystem generic fs_meta/file_node lookup/inject/pack function as a
>> callback, such as
>>  * .find_fs_sb
>>  * .inject_fs_sb
>>  * .pack_fs_sb
> 
> What about group descriptors?  AG headers?  The AGFLWTFBBQLOL?
> 
>>  * .find_fs_bitmap
>>  * .inject_fs_bitmap
> 
> Probably want an find/inject for log blocks too.
> 
> Oh, wait, XFS doesn't log blocks like jbd2 does. :) :)

Yes, I admit that I should miss a lot of fs meta type here, but that's just a
simple example here, we should not treat it as a full design :)

> 
>>  * .find_fs_inode_bitmap
>>  * .inject_fs_inode_bitmap
> 
> XFS has an inode bitmap? ;)

We can leave callback as NULL? ;)

> 
> (This is why there's no generic fuzz tool; every fs is different enough
> that doing so would be sort of a mess.)

Yes, I just wonder if there is any possible we can save some redundant work.

> 
> ((Granted, you could also look at how xfstests uses the xfs_db fuzz
> command so at least it would be systematic...))
Okay, I will check that.

Thanks,

> 
>>  * .find_inode_by_num
>>  * .inject_inode
>>  * .pack_inode
>>  * .find_tree_node_by_level
>> ...
> 
> What about the name/value btrees?  (Ok, I'll stop now.)
> 
> --D
> 
>> then specific filesystem can fill the callback to tell how the tool can 
>> locate a
>> field in inode or a metadata in tree node and then trigger the designed fuzz.
>>
>> It will be easier to rewrite whole generic fwk for each filesystem, because
>> existed filesystem userspace tool should has included above callback's detail
>> codes...
>>
>>> On Tue, Aug 20, 2019 at 10:24:11AM +0800, Chao Yu wrote:
>>>> filesystem fill the tool's callback to seek a node/block and supported 
>>>> fields
>>>> can be fuzzed in inode.
>>
>>>
>>> What you *can* do is to make the file system specific portion of the
>>> work as small as possible.  Great work in this area is Professor Kim's
>>> Janus[1][2] and Hydra[2] work.  (Hydra is about to be published at SOSP 19,
>>> and was partially funded from a Google Faculty Research Work.)
>>>
>>> [1] https://taesoo.kim/pubs/2019/xu:janus.pdf
>>> [2] https://github.com/sslab-gatech/janus
>>> [3] https://github.com/sslab-gatech/hydra
>>
>> Thanks for the information!
>>
>> It looks like janus and hydra alreay have generic compress/decompress 
>> function
>> across different filesystems, it's really a good job, I do think it may be 
>> the
>> one once it becomes more generic.
>>
>> Thanks
>>
>>>
> .
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] erofs: move erofs out of staging

[Chao Yu]

On 2019/8/20 23:56, Theodore Y. Ts'o wrote:
> The reason why there needs to be at least some file system specific
> code for fuzz testing is because for efficiency's sake, you don't want
> to fuzz every single bit in the file system, but just the ones which
> are most interesting (e.g., the metadata blocks).  For file systems
> which use checksum to protect against accidental corruption, the file
> system fuzzer needs to also fix up the checksums (since you can be
> sure malicious attackers will do this).

Yup, IMO, if we really want such tool, it needs to:
- move all generic fuzz codes (trigger random fuzzing in meta/data area) into
that tool, and
- make filesystem generic fs_meta/file_node lookup/inject/pack function as a
callback, such as
 * .find_fs_sb
 * .inject_fs_sb
 * .pack_fs_sb
 * .find_fs_bitmap
 * .inject_fs_bitmap
 * .find_fs_inode_bitmap
 * .inject_fs_inode_bitmap
 * .find_inode_by_num
 * .inject_inode
 * .pack_inode
 * .find_tree_node_by_level
...
then specific filesystem can fill the callback to tell how the tool can locate a
field in inode or a metadata in tree node and then trigger the designed fuzz.

It will be easier to rewrite whole generic fwk for each filesystem, because
existed filesystem userspace tool should has included above callback's detail
codes...

> On Tue, Aug 20, 2019 at 10:24:11AM +0800, Chao Yu wrote:
>> filesystem fill the tool's callback to seek a node/block and supported fields
>> can be fuzzed in inode.

> 
> What you *can* do is to make the file system specific portion of the
> work as small as possible.  Great work in this area is Professor Kim's
> Janus[1][2] and Hydra[2] work.  (Hydra is about to be published at SOSP 19,
> and was partially funded from a Google Faculty Research Work.)
> 
> [1] https://taesoo.kim/pubs/2019/xu:janus.pdf
> [2] https://github.com/sslab-gatech/janus
> [3] https://github.com/sslab-gatech/hydra

Thanks for the information!

It looks like janus and hydra alreay have generic compress/decompress function
across different filesystems, it's really a good job, I do think it may be the
one once it becomes more generic.

Thanks

> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] erofs: move erofs out of staging

[Chao Yu]

On 2019/8/20 10:38, Qu Wenruo wrote:
> 
> 
> On 2019/8/20 上午10:24, Chao Yu wrote:
>> On 2019/8/20 8:55, Qu Wenruo wrote:
>>> [...]
>>>>>> I have made a simple fuzzer to inject messy in inode metadata,
>>>>>> dir data, compressed indexes and super block,
>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/xiang/erofs-utils.git/commit/?h=experimental-fuzzer
>>>>>>
>>>>>> I am testing with some given dirs and the following script.
>>>>>> Does it look reasonable?
>>>>>>
>>>>>> # !/bin/bash
>>>>>>
>>>>>> mkdir -p mntdir
>>>>>>
>>>>>> for ((i=0; i<1000; ++i)); do
>>>>>>  mkfs/mkfs.erofs -F$i testdir_fsl.fuzz.img testdir_fsl > /dev/null 2>&1
>>>>>
>>>>> mkfs fuzzes the image? Er
>>>>
>>>> Thanks for your reply.
>>>>
>>>> First, This is just the first step of erofs fuzzer I wrote yesterday 
>>>> night...
>>>>
>>>>>
>>>>> Over in XFS land we have an xfs debugging tool (xfs_db) that knows how
>>>>> to dump (and write!) most every field of every metadata type.  This
>>>>> makes it fairly easy to write systematic level 0 fuzzing tests that
>>>>> check how well the filesystem reacts to garbage data (zeroing,
>>>>> randomizing, oneing, adding and subtracting small integers) in a field.
>>>>> (It also knows how to trash entire blocks.)
>>>
>>> The same tool exists for btrfs, although lacks the write ability, but
>>> that dump is more comprehensive and a great tool to learn the on-disk
>>> format.
>>>
>>>
>>> And for the fuzzing defending part, just a few kernel releases ago,
>>> there is none for btrfs, and now we have a full static verification
>>> layer to cover (almost) all on-disk data at read and write time.
>>> (Along with enhanced runtime check)
>>>
>>> We have covered from vague values inside tree blocks and invalid/missing
>>> cross-ref find at runtime.
>>>
>>> Currently the two layered check works pretty fine (well, sometimes too
>>> good to detect older, improper behaved kernel).
>>> - Tree blocks with vague data just get rejected by verification layer
>>>   So that all members should fit on-disk format, from alignment to
>>>   generation to inode mode.
>>>
>>>   The error will trigger a good enough (TM) error message for developer
>>>   to read, and if we have other copies, we retry other copies just as
>>>   we hit a bad copy.
>>>
>>> - At runtime, we have much less to check
>>>   Only cross-ref related things can be wrong now. since everything
>>>   inside a single tree block has already be checked.
>>>
>>> In fact, from my respect of view, such read time check should be there
>>> from the very beginning.
>>> It acts kinda of a on-disk format spec. (In fact, by implementing the
>>> verification layer itself, it already exposes a lot of btrfs design
>>> trade-offs)
>>>
>>> Even for a fs as complex (buggy) as btrfs, we only take 1K lines to
>>> implement the verification layer.
>>> So I'd like to see every new mainlined fs to have such ability.
>>
>> Out of curiosity, it looks like every mainstream filesystem has its own
>> fuzz/injection tool in their tool-set, if it's really such a generic
>> requirement, why shouldn't there be a common tool to handle that, let 
>> specified
>> filesystem fill the tool's callback to seek a node/block and supported fields
>> can be fuzzed in inode.
> 
> It could be possible for XFS/EXT* to share the same infrastructure
> without much hassle.
> (If not considering external journal)
> 
> But for btrfs, it's like a regular fs on a super large dm-linear, which
> further builds its chunks on different dm-raid1/dm-linear/dm-raid56.
> 
> So not sure if it's possible for btrfs, as it contains its logical
> address layer bytenr (the most common one) along with per-chunk physical
> mapping bytenr (in another tree).

Yeah, it looks like we need searching more levels mapping to find the final
physical block address of inode/node/data in btrfs.

IMO, in a little lazy way, we can reform and reuse existed function in
btrfs-progs which can find the mapping info of inode/node/data according to
specified ino or ino+pg_no.

> 
> It may depends on the granularity. But definitely a good idea to do so
> in a g

Re: [PATCH] erofs: move erofs out of staging

[Chao Yu]

On 2019/8/20 8:55, Qu Wenruo wrote:
> [...]
 I have made a simple fuzzer to inject messy in inode metadata,
 dir data, compressed indexes and super block,
 https://git.kernel.org/pub/scm/linux/kernel/git/xiang/erofs-utils.git/commit/?h=experimental-fuzzer

 I am testing with some given dirs and the following script.
 Does it look reasonable?

 # !/bin/bash

 mkdir -p mntdir

 for ((i=0; i<1000; ++i)); do
mkfs/mkfs.erofs -F$i testdir_fsl.fuzz.img testdir_fsl > /dev/null 2>&1
>>>
>>> mkfs fuzzes the image? Er
>>
>> Thanks for your reply.
>>
>> First, This is just the first step of erofs fuzzer I wrote yesterday night...
>>
>>>
>>> Over in XFS land we have an xfs debugging tool (xfs_db) that knows how
>>> to dump (and write!) most every field of every metadata type.  This
>>> makes it fairly easy to write systematic level 0 fuzzing tests that
>>> check how well the filesystem reacts to garbage data (zeroing,
>>> randomizing, oneing, adding and subtracting small integers) in a field.
>>> (It also knows how to trash entire blocks.)
> 
> The same tool exists for btrfs, although lacks the write ability, but
> that dump is more comprehensive and a great tool to learn the on-disk
> format.
> 
> 
> And for the fuzzing defending part, just a few kernel releases ago,
> there is none for btrfs, and now we have a full static verification
> layer to cover (almost) all on-disk data at read and write time.
> (Along with enhanced runtime check)
> 
> We have covered from vague values inside tree blocks and invalid/missing
> cross-ref find at runtime.
> 
> Currently the two layered check works pretty fine (well, sometimes too
> good to detect older, improper behaved kernel).
> - Tree blocks with vague data just get rejected by verification layer
>   So that all members should fit on-disk format, from alignment to
>   generation to inode mode.
> 
>   The error will trigger a good enough (TM) error message for developer
>   to read, and if we have other copies, we retry other copies just as
>   we hit a bad copy.
> 
> - At runtime, we have much less to check
>   Only cross-ref related things can be wrong now. since everything
>   inside a single tree block has already be checked.
> 
> In fact, from my respect of view, such read time check should be there
> from the very beginning.
> It acts kinda of a on-disk format spec. (In fact, by implementing the
> verification layer itself, it already exposes a lot of btrfs design
> trade-offs)
> 
> Even for a fs as complex (buggy) as btrfs, we only take 1K lines to
> implement the verification layer.
> So I'd like to see every new mainlined fs to have such ability.

Out of curiosity, it looks like every mainstream filesystem has its own
fuzz/injection tool in their tool-set, if it's really such a generic
requirement, why shouldn't there be a common tool to handle that, let specified
filesystem fill the tool's callback to seek a node/block and supported fields
can be fuzzed in inode. It can help to avoid redundant work whenever Linux
welcomes a new filesystem

Thanks,

> 
>>
>> Actually, compared with XFS, EROFS has rather simple on-disk format.
>> What we inject one time is quite deterministic.
>>
>> The first step just purposely writes some random fuzzed data to
>> the base inode metadata, compressed indexes, or dir data field
>> (one round one field) to make it validity and coverability.
>>
>>>
>>> You might want to write such a debugging tool for erofs so that you can
>>> take apart crashed images to get a better idea of what went wrong, and
>>> to write easy fuzzing tests.
>>
>> Yes, we will do such a debugging tool of course. Actually Li Guifu is now
>> developping a erofs-fuse to support old linux versions or other OSes for
>> archiveing only use, we will base on that code to develop a better fuzzer
>> tool as well.
> 
> Personally speaking, debugging tool is way more important than a running
> kernel module/fuse.
> It's human trying to write the code, most of time is spent educating
> code readers, thus debugging tool is way more important than dead cold code.
> 
> Thanks,
> Qu
>>
>> Thanks,
>> Gao Xiang
>>
>>>
>>> --D
>>>
umount mntdir
mount -t erofs -o loop testdir_fsl.fuzz.img mntdir
for j in `find mntdir -type f`; do
md5sum $j > /dev/null
done
 done

 Thanks,
 Gao Xiang

>
> Thanks,
> Gao Xiang
>
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 6/6] staging: erofs: avoid endless loop of invalid lookback distance 0

[Chao Yu]

On 2019-8-19 18:34, Gao Xiang wrote:
> As reported by erofs-utils fuzzer, Lookback distance should
> be a positive number, so it should be actually looked back
> rather than spinning.
> 
> Fixes: 02827e1796b3 ("staging: erofs: add erofs_map_blocks_iter")
> Cc:  # 4.19+
> Signed-off-by: Gao Xiang 

Reviewed-by: Chao Yu 

Thanks,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 5/6] staging: erofs: detect potential multiref due to corrupted images

[Chao Yu]

On 2019-8-19 18:34, Gao Xiang wrote:
> As reported by erofs-utils fuzzer, currently, multiref
> (ondisk deduplication) hasn't been supported for now,
> we should forbid it properly.
> 
> Fixes: 3883a79abd02 ("staging: erofs: introduce VLE decompression support")
> Cc:  # 4.19+
> Signed-off-by: Gao Xiang 
> ---
>  drivers/staging/erofs/zdata.c | 16 ++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/erofs/zdata.c b/drivers/staging/erofs/zdata.c
> index aae2f2b8353f..5b6fef5181af 100644
> --- a/drivers/staging/erofs/zdata.c
> +++ b/drivers/staging/erofs/zdata.c
> @@ -816,8 +816,16 @@ static int z_erofs_decompress_pcluster(struct 
> super_block *sb,
>   pagenr = z_erofs_onlinepage_index(page);
>  
>   DBG_BUGON(pagenr >= nr_pages);
> - DBG_BUGON(pages[pagenr]);
>  
> + /*
> +  * currently EROFS doesn't support multiref(dedup),
> +  * so here erroring out one multiref page.
> +  */
> + if (unlikely(pages[pagenr])) {
> + DBG_BUGON(1);
> + SetPageError(pages[pagenr]);
> + z_erofs_onlinepage_endio(pages[pagenr]);

Should set err meanwhile?

> + }
>   pages[pagenr] = page;
>   }
>   z_erofs_pagevec_ctor_exit(, true);
> @@ -849,7 +857,11 @@ static int z_erofs_decompress_pcluster(struct 
> super_block *sb,
>   pagenr = z_erofs_onlinepage_index(page);
>  
>   DBG_BUGON(pagenr >= nr_pages);
> - DBG_BUGON(pages[pagenr]);
> + if (unlikely(pages[pagenr])) {
> + DBG_BUGON(1);
> + SetPageError(pages[pagenr]);
> + z_erofs_onlinepage_endio(pages[pagenr]);
> + }
>   pages[pagenr] = page;
>  
>   overlapped = true;
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 4/6] staging: erofs: avoid loop in submit chains

[Chao Yu]

On 2019-8-19 18:34, Gao Xiang wrote:
> As reported by erofs-utils fuzzer, 2 conditions
> can happen in corrupted images, which can cause
> unexpected behaviors.
>  - access the same pcluster one more time;
>  - access the tail end pcluster again, e.g.
> _ access again (will trigger tail merging)
>|
>  1 2 3 1 2 ->   1 2 3 1
>  |_ tail end of the chain\___/ (unexpected behavior)
> Let's detect and avoid them now.
> 
> Signed-off-by: Gao Xiang 

Reviewed-by: Chao Yu 

Thanks,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/6] staging: erofs: cannot set EROFS_V_Z_INITED_BIT if fill_inode_lazy fails

[Chao Yu]

On 2019-8-19 18:34, Gao Xiang wrote:
> As reported by erofs-utils fuzzer, unsupported compressed
> clustersize will make fill_inode_lazy fail, for such case
> we cannot set EROFS_V_Z_INITED_BIT since we need return
> failure for each z_erofs_map_blocks_iter().
> 
> Fixes: 152a333a5895 ("staging: erofs: add compacted compression indexes 
> support")
> Cc:  # 5.3+
> Signed-off-by: Gao Xiang 

Reviewed-by: Chao Yu 

Thanks,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 3/6] staging: erofs: add two missing erofs_workgroup_put for corrupted images

[Chao Yu]

On 2019-8-19 18:34, Gao Xiang wrote:
> As reported by erofs-utils fuzzer, these error handling
> path will be entered to handle corrupted images.
> 
> Lack of erofs_workgroup_puts will cause unmounting
> unsuccessfully.
> 
> Fix these return values to EFSCORRUPTED as well.
> 
> Fixes: 3883a79abd02 ("staging: erofs: introduce VLE decompression support")
> Cc:  # 4.19+
> Signed-off-by: Gao Xiang 

Reviewed-by: Chao Yu 

Thanks,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/6] staging: erofs: some compressed cluster should be submitted for corrupted images

[Chao Yu]

On 2019-8-19 18:34, Gao Xiang wrote:
> As reported by erofs_utils fuzzer, a logical page can belong
> to at most 2 compressed clusters, if one compressed cluster
> is corrupted, but the other has been ready in submitting chain.
> 
> The chain needs to submit anyway in order to keep the page
> working properly (page unlocked with PG_error set, PG_uptodate
> not set).
> 
> Let's fix it now.
> 
> Fixes: 3883a79abd02 ("staging: erofs: introduce VLE decompression support")
> Cc:  # 4.19+
> Signed-off-by: Gao Xiang 

Reviewed-by: Chao Yu 

Thanks,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/6] staging: erofs: some compressed cluster should be submitted for corrupted images

[Chao Yu]

On 2019-8-19 18:34, Gao Xiang wrote:
> As reported by erofs_utils fuzzer, a logical page can belong
> to at most 2 compressed clusters, if one compressed cluster
> is corrupted, but the other has been ready in submitting chain.
> 
> The chain needs to submit anyway in order to keep the page
> working properly (page unlocked with PG_error set, PG_uptodate
> not set).
> 
> Let's fix it now.
> 
> Fixes: 3883a79abd02 ("staging: erofs: introduce VLE decompression support")
> Cc:  # 4.19+
> Signed-off-by: Gao Xiang 

Reviewed-by: Chao Yu 

Thanks,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: erofs: refuse to mount images with malformed volume name

[Chao Yu]

On 2019-8-18 18:28, Gao Xiang wrote:
> From: Gao Xiang 
> 
> As Richard reminder [1], A valid volume name should be
> ended in NIL terminator within the length of volume_name.
> 
> Since this field currently isn't really used, let's fix
> it to avoid potential bugs in the future.
> 
> [1] 
> https://lore.kernel.org/r/1133002215.69049.1566119033047.javamail.zim...@nod.at/
> 
> Reported-by: Richard Weinberger 
> Signed-off-by: Gao Xiang 

Reviewed-by: Chao Yu 

Thanks,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 RESEND] staging: erofs: fix an error handling in erofs_readdir()

[Chao Yu]

On 2019-8-18 11:21, Gao Xiang wrote:
> From: Gao Xiang 
> 
> Richard observed a forever loop of erofs_read_raw_page() [1]
> which can be generated by forcely setting ->u.i_blkaddr
> to 0xdeadbeef (as my understanding block layer can
> handle access beyond end of device correctly).
> 
> After digging into that, it seems the problem is highly
> related with directories and then I found the root cause
> is an improper error handling in erofs_readdir().
> 
> Let's fix it now.
> 
> [1] 
> https://lore.kernel.org/r/1163995781.68824.1566084358245.javamail.zim...@nod.at/
> 
> Reported-by: Richard Weinberger 
> Fixes: 3aa8ec716e52 ("staging: erofs: add directory operations")
> Cc:  # 4.19+
> Signed-off-by: Gao Xiang 

Reviewed-by: Chao Yu 

Thanks,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] staging: erofs: fix an error handling in erofs_readdir()

[Chao Yu]

Hi Xiang,

On 2019-8-18 18:52, Gao Xiang wrote:
> Hi Chao,
> 
> On Sun, Aug 18, 2019 at 06:39:52PM +0800, Chao Yu wrote:
>> On 2019-8-18 10:53, Matthew Wilcox wrote:
>>> On Sun, Aug 18, 2019 at 10:32:45AM +0800, Gao Xiang wrote:
>>>> On Sat, Aug 17, 2019 at 07:20:55PM -0700, Matthew Wilcox wrote:
>>>>> On Sun, Aug 18, 2019 at 09:56:31AM +0800, Gao Xiang wrote:
>>>>>> @@ -82,8 +82,12 @@ static int erofs_readdir(struct file *f, struct 
>>>>>> dir_context *ctx)
>>>>>>  unsigned int nameoff, maxsize;
>>>>>>  
>>>>>>  dentry_page = read_mapping_page(mapping, i, NULL);
>>>>>> -if (IS_ERR(dentry_page))
>>>>>> -continue;
>>>>>> +if (IS_ERR(dentry_page)) {
>>>>>> +errln("fail to readdir of logical block %u of 
>>>>>> nid %llu",
>>>>>> +  i, EROFS_V(dir)->nid);
>>>>>> +err = PTR_ERR(dentry_page);
>>>>>> +break;
>>>>>
>>>>> I don't think you want to use the errno that came back from
>>>>> read_mapping_page() (which is, I think, always going to be -EIO).
>>>>> Rather you want -EFSCORRUPTED, at least if I understand the recent
>>>>> patches to ext2/ext4/f2fs/xfs/...
>>>>
>>>> Thanks for your reply and noticing this. :)
>>>>
>>>> Yes, as I talked with you about read_mapping_page() in a xfs related
>>>> topic earlier, I think I fully understand what returns here.
>>>>
>>>> I actually had some concern about that before sending out this patch.
>>>> You know the status is
>>>>PG_uptodate is not set and PG_error is set here.
>>>>
>>>> But we cannot know it is actually a disk read error or due to
>>>> corrupted images (due to lack of page flags or some status, and
>>>> I think it could be a waste of page structure space for such
>>>> corrupted image or disk error)...
>>>>
>>>> And some people also like propagate errors from insiders...
>>>> (and they could argue about err = -EFSCORRUPTED as well..)
>>>>
>>>> I'd like hear your suggestion about this after my words above?
>>>> still return -EFSCORRUPTED?
>>>
>>> I don't think it matters whether it's due to a disk error or a corrupted
>>> image.  We can't read the directory entry, so we should probably return
>>> -EFSCORRUPTED.  Thinking about it some more, read_mapping_page() can
>>> also return -ENOMEM, so it should probably look something like this:
>>>
>>> err = 0;
>>> if (dentry_page == ERR_PTR(-ENOMEM))
>>> err = -ENOMEM;
>>> else if (IS_ERR(dentry_page)) {
>>> errln("fail to readdir of logical block %u of nid %llu",
>>>   i, EROFS_V(dir)->nid);
>>> err = -EFSCORRUPTED;
>>
>> Well, if there is real IO error happen under filesystem, we should return 
>> -EIO
>> instead of EFSCORRUPTED?
>>
>> The right fix may be that doing sanity check on on-disk blkaddr, and return
>> -EFSCORRUPTED if the blkaddr is invalid and propagate the error to its caller
>> erofs_readdir(), IIUC below error info.
> 
> In my thought, I actually don't care what is actually returned
> (In other words, I have no tendency about EFSCORRUPTED / EIO)
> as long as it behaves normal for corrupted image.

I doubt that user can and be willing to handle different error code in there
error path.

> 
> A little concern is that I have no idea whether all user problems
> can handle EUCLEAN properly.

Yes, I can see it's widely used in fileystem, I guess it would be better to
update manual of common fs interfaces to let user be aware of the meaning of it.

> 
> I don't want to limit blkaddr as what ->blocks recorded in
> erofs_super_block since it's already used for our hotpatching
> approach and that field is only used for statfs() for users
> to know its visible size, and block layer will block such
> invalid block access.
> 
> All in all, that is minor. I think we can fix as what Matthew said.

Yeah, as we discuss offline, we need keep flexibility on current version for
android, and maybe we can add a feature to check blkaddr validation later.

Thanks,

> 
> Thanks,
> Gao Xiang
> 
>>
>>> [36297.354090] attempt to access beyond end of device
>>> [36297.354098] loop17: rw=0, want=29887428984, limit=1953128
>>> [36297.354107] attempt to access beyond end of device
>>> [36297.354109] loop17: rw=0, want=29887428480, limit=1953128
>>> [36301.827234] attempt to access beyond end of device
>>> [36301.827243] loop17: rw=0, want=29887428480, limit=1953128
>>
>> Thanks,
>>
>>> }
>>>
>>> if (err)
>>> break;
>>>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] staging: erofs: fix an error handling in erofs_readdir()

[Chao Yu]

On 2019-8-18 10:53, Matthew Wilcox wrote:
> On Sun, Aug 18, 2019 at 10:32:45AM +0800, Gao Xiang wrote:
>> On Sat, Aug 17, 2019 at 07:20:55PM -0700, Matthew Wilcox wrote:
>>> On Sun, Aug 18, 2019 at 09:56:31AM +0800, Gao Xiang wrote:
 @@ -82,8 +82,12 @@ static int erofs_readdir(struct file *f, struct 
 dir_context *ctx)
unsigned int nameoff, maxsize;
  
dentry_page = read_mapping_page(mapping, i, NULL);
 -  if (IS_ERR(dentry_page))
 -  continue;
 +  if (IS_ERR(dentry_page)) {
 +  errln("fail to readdir of logical block %u of nid %llu",
 +i, EROFS_V(dir)->nid);
 +  err = PTR_ERR(dentry_page);
 +  break;
>>>
>>> I don't think you want to use the errno that came back from
>>> read_mapping_page() (which is, I think, always going to be -EIO).
>>> Rather you want -EFSCORRUPTED, at least if I understand the recent
>>> patches to ext2/ext4/f2fs/xfs/...
>>
>> Thanks for your reply and noticing this. :)
>>
>> Yes, as I talked with you about read_mapping_page() in a xfs related
>> topic earlier, I think I fully understand what returns here.
>>
>> I actually had some concern about that before sending out this patch.
>> You know the status is
>>PG_uptodate is not set and PG_error is set here.
>>
>> But we cannot know it is actually a disk read error or due to
>> corrupted images (due to lack of page flags or some status, and
>> I think it could be a waste of page structure space for such
>> corrupted image or disk error)...
>>
>> And some people also like propagate errors from insiders...
>> (and they could argue about err = -EFSCORRUPTED as well..)
>>
>> I'd like hear your suggestion about this after my words above?
>> still return -EFSCORRUPTED?
> 
> I don't think it matters whether it's due to a disk error or a corrupted
> image.  We can't read the directory entry, so we should probably return
> -EFSCORRUPTED.  Thinking about it some more, read_mapping_page() can
> also return -ENOMEM, so it should probably look something like this:
> 
>   err = 0;
>   if (dentry_page == ERR_PTR(-ENOMEM))
>   err = -ENOMEM;
>   else if (IS_ERR(dentry_page)) {
>   errln("fail to readdir of logical block %u of nid %llu",
> i, EROFS_V(dir)->nid);
>   err = -EFSCORRUPTED;

Well, if there is real IO error happen under filesystem, we should return -EIO
instead of EFSCORRUPTED?

The right fix may be that doing sanity check on on-disk blkaddr, and return
-EFSCORRUPTED if the blkaddr is invalid and propagate the error to its caller
erofs_readdir(), IIUC below error info.

> [36297.354090] attempt to access beyond end of device
> [36297.354098] loop17: rw=0, want=29887428984, limit=1953128
> [36297.354107] attempt to access beyond end of device
> [36297.354109] loop17: rw=0, want=29887428480, limit=1953128
> [36301.827234] attempt to access beyond end of device
> [36301.827243] loop17: rw=0, want=29887428480, limit=1953128

Thanks,

>   }
> 
>   if (err)
>   break;
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] erofs: move erofs out of staging

[Chao Yu]

Hi Richard,

On 2019-8-18 17:21, Richard Weinberger wrote:
> For normal use I see no problem at all.
> I fear distros that try to mount anything you plug into your USB.
> 
> At least SUSE already blacklists erofs:
> https://github.com/openSUSE/suse-module-tools/blob/master/suse-module-tools.spec#L24

Thanks for letting us know current status of erofs in SUSE distro.

Currently erofs cares more about the requirement of Android, in there, we are
safe on fuzzed image case as dm-verity can keep all partition data being
verified before mount.

For other scenarios, like distro, erofs should improve itself step by step as
many mainline filesystems in many aspects to fit the there-in requirement. :)

Thanks,

> 
> Thanks,
> //richard
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: erofs: use common file type conversion

[Chao Yu]

On 2019/8/16 15:11, Gao Xiang wrote:
> Deduplicate the EROFS file type conversion implementation and
> remove EROFS_FT_* definitions since it's the same as defined
> by POSIX, let's follow ext2 as Linus pointed out [1]
> commit e10892189428 ("ext2: use common file type conversion").
> 
> [1] 
> https://lore.kernel.org/r/CAHk-=wiUs+b=ivkm3mvooxgvk7cmmc67ktmnaul0cd_cmmv...@mail.gmail.com/
> 
> Reported-by: Linus Torvalds 
> Signed-off-by: Gao Xiang 

Reviewed-by: Chao Yu 

Thanks,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 3/3] staging: erofs: correct all misused ENOTSUPP

[Chao Yu]

On 2019/8/14 18:37, Gao Xiang wrote:
> As Chao pointed out [1], ENOTSUPP is used for NFS
> protocol only, we should use EOPNOTSUPP instead...
> 
> [1] 
> https://lore.kernel.org/lkml/108ee2f9-75dd-b8ab-8da7-b81c17baf...@huawei.com/
> 
> Reported-by: Chao Yu 
> Signed-off-by: Gao Xiang 

Reviewed-by: Chao Yu 

Thanks,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH RESEND 2/2] staging: erofs: differentiate unsupported on-disk format

[Chao Yu]

On 2019/8/14 12:32, Gao Xiang wrote:
> For some specific fields, use ENOTSUPP instead of EIO
> for values which look sane but aren't supported right now.
> 
> Signed-off-by: Gao Xiang 

Reviewed-by: Chao Yu 

> + return -ENOTSUPP;

A little bit confused about when we need to use ENOTSUPP or EOPNOTSUPP, I
checked several manual of syscall, it looks EOPNOTSUPP is widely used.

Thanks,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH RESEND 1/2] staging: erofs: introduce EFSCORRUPTED and more logs

[Chao Yu]

On 2019/8/14 12:32, Gao Xiang wrote:
> Previously, EROFS uses EIO to indicate that filesystem is
> corrupted as well, but other filesystems tend to use
> EUCLEAN instead, let's follow what others do right now.
> 
> Also, add some more prints to the syslog.
> 
> Suggested-by: Pavel Machek 
> Signed-off-by: Gao Xiang 

Reviewed-by: Chao Yu 

Thanks,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: erofs: removing an extra call to iloc() in fill_inode()

[Chao Yu]

On 2019/8/14 11:52, Pratik Shinde wrote:
> Yes.since we already have a function with same name (and we are using it in 
> same
> context).
> 'inode_loc' was the most meaningful name I could come up with :)

[snip]

On Wed, Aug 14, 2019 at 7:37 AM Gao Xiang  wrote:
> iloc is the name of static inline helper function in internal.h
> used for shorter lines...

Correct, so let's keep as it is.

Thanks,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: erofs: removing an extra call to iloc() in fill_inode()

[Chao Yu]

On 2019/8/14 9:59, Gao Xiang wrote:
> Hi Pratik,
> 
> On Wed, Aug 14, 2019 at 02:08:40AM +0530, Pratik Shinde wrote:
>> in fill_inode() we call iloc() twice.Avoiding the extra call by
>> storing the result.
>>
>> Signed-off-by: Pratik Shinde 
> 
> I have no objection of this patch, but I'd like to
> hear Chao/Greg's idea about this...

It looks more clean. :)

Nitpick, maybe change 'inode_loc' to shorter 'iloc' will be better.

Reviewed-by: Chao Yu 

Thanks,

> 
> Thanks,
> Gao Xiang
> 
>> ---
>>  drivers/staging/erofs/inode.c | 7 ---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/staging/erofs/inode.c b/drivers/staging/erofs/inode.c
>> index 4c3d8bf..d82ba6c 100644
>> --- a/drivers/staging/erofs/inode.c
>> +++ b/drivers/staging/erofs/inode.c
>> @@ -167,11 +167,12 @@ static int fill_inode(struct inode *inode, int isdir)
>>  int err;
>>  erofs_blk_t blkaddr;
>>  unsigned int ofs;
>> +erofs_off_t inode_loc;
>>  
>>  trace_erofs_fill_inode(inode, isdir);
>> -
>> -blkaddr = erofs_blknr(iloc(sbi, vi->nid));
>> -ofs = erofs_blkoff(iloc(sbi, vi->nid));
>> +inode_loc = iloc(sbi, vi->nid);
>> +blkaddr = erofs_blknr(inode_loc);
>> +ofs = erofs_blkoff(inode_loc);
>>  
>>  debugln("%s, reading inode nid %llu at %u of blkaddr %u",
>>  __func__, vi->nid, ofs, blkaddr);
>> -- 
>> 2.9.3
>>
> .
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 3/3] staging: erofs: xattr.c: avoid BUG_ON

[Chao Yu]

Hi Xiang,

On 2019/8/13 11:57, Gao Xiang wrote:
> Hi Chao,
> 
> On Tue, Aug 13, 2019 at 11:20:22AM +0800, Chao Yu wrote:
>> On 2019/8/13 10:30, Gao Xiang wrote:
>>> Kill all the remaining BUG_ON in EROFS:
>>>  - one BUG_ON was used to detect xattr on-disk corruption,
>>>proper error handling should be added for it instead;
>>>  - the other BUG_ONs are used to detect potential issues,
>>>use DBG_BUGON only in (eng) debugging version.
>>
>> BTW, do we need add WARN_ON() into DBG_BUGON() to show some details function 
>> or
>> call stack in where we encounter the issue?
> 
> Thanks for kindly review :)
> 
> Agreed, it seems much better. If there are no other considerations
> here, I can submit another patch addressing it later or maybe we
> can change it in the next linux version since I'd like to focusing
> on moving out of staging for this round...

No problem, we can change it in a proper time.

Thanks,

> 
> Thanks,
> Gao Xiang
> 
>>
>>>
>>> Signed-off-by: Gao Xiang 
>>
>> Reviewed-by: Chao Yu 
>>
>> Thanks,
> .
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 3/3] staging: erofs: xattr.c: avoid BUG_ON

[Chao Yu]

On 2019/8/13 10:30, Gao Xiang wrote:
> Kill all the remaining BUG_ON in EROFS:
>  - one BUG_ON was used to detect xattr on-disk corruption,
>proper error handling should be added for it instead;
>  - the other BUG_ONs are used to detect potential issues,
>use DBG_BUGON only in (eng) debugging version.

BTW, do we need add WARN_ON() into DBG_BUGON() to show some details function or
call stack in where we encounter the issue?

> 
> Signed-off-by: Gao Xiang 

Reviewed-by: Chao Yu 

Thanks,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/3] staging: erofs: remove incomplete cleancache

[Chao Yu]

On 2019/8/13 10:30, Gao Xiang wrote:
> cleancache was not fully implemented in EROFS.
> In addition, it's tend to remove the whole cleancache in
> related attempt [1].
> 
> [1] 
> https://lore.kernel.org/linux-fsdevel/20190527103207.13287-3-jgr...@suse.com/
> Signed-off-by: Gao Xiang 

Reviewed-by: Chao Yu 

Thanks,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/3] staging: erofs: inline erofs_inode_is_data_compressed()

[Chao Yu]

On 2019/8/13 10:30, Gao Xiang wrote:
> As a helper in erofs_fs.h, erofs_inode_is_data_compressed()
> should be inlined.
> 
> Signed-off-by: Gao Xiang 

Reviewed-by: Chao Yu 

Thanks,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 12/22] staging: erofs: drop __GFP_NOFAIL for managed inode

[Chao Yu]

On 2019/7/31 23:57, Gao Xiang wrote:
> For historical reasons, __GFP_NOFAIL was set for managed inode.
> It's no need using that since EROFS can handle it properly.
> 
> Signed-off-by: Gao Xiang 

Reviewed-by: Chao Yu 

Thanks,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 08/22] staging: erofs: kill CONFIG_EROFS_FS_IO_MAX_RETRIES

[Chao Yu]

On 2019/7/31 23:57, Gao Xiang wrote:
> CONFIG_EROFS_FS_IO_MAX_RETRIES seems a runtime setting
> and users have no idea about the change in behaviour.
> 
> Let's remove the setting currently and could turn it
> into a module parameter if it's really needed.
> 
> Suggested-by: David Sterba 
> Signed-off-by: Gao Xiang 

Reviewed-by: Chao Yu 

Thanks,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 07/22] staging: erofs: remove redundant #include "internal.h"

[Chao Yu]

On 2019/7/31 23:57, Gao Xiang wrote:
> Because #include "internal.h" is included in xattr.h
> 
> Signed-off-by: Gao Xiang 

Reviewed-by: Chao Yu 

Thanks,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 07/22] staging: erofs: remove redundant #include "internal.h"

[Chao Yu]

On 2019/7/31 20:54, Gao Xiang wrote:
> 
> 
> On 2019/7/31 20:07, Chao Yu wrote:
>> Hi Xiang,
>>
>> On 2019/7/31 15:08, Gao Xiang wrote:
>>> Hi Chao,
>>>
>>> On 2019/7/31 15:03, Chao Yu wrote:
>>>> On 2019/7/29 14:51, Gao Xiang wrote:
>>>>> Because #include "internal.h" is included in xattr.h
>>>>
>>>> I think it would be better to remove "internal.h" in xattr.h, and include 
>>>> them
>>>> both in .c file in where we need xattr definition.
>>>
>>> It seems that all xattr related source files needing internal.h,
>>> and we need "EROFS_V(inode)", "struct erofs_sb_info", ... stuffs in xattr.h,
>>> which is defined in internal.h...
>>
>> Since I checked f2fs', it looks it's okay to don't include internal.h for
>> xattr.h, if .c needs xattr.h, we can just include interanl.h and xattr.h in 
>> the
>> head of it, it's safe.
> 
> I think xattr.h should be used independently (all dependencies of xattr.h 
> should
> be included in xattr.h, most of include files behave like that)... Maybe it is
> not a good way to follow f2fs...

Yes, I've confirmed it's fine to do this, let's go ahead. :)

Thanks,

> 
> Thanks,
> Gao Xiang
> 
>>
>> Thanks,
>>
>>>
>>> Thanks,
>>> Gao Xiang
>>>
>>>>
>>>> Thanks,
>>>>
>>> .
>>>
> .
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 08/22] staging: erofs: kill CONFIG_EROFS_FS_IO_MAX_RETRIES

[Chao Yu]

Hi Xiang,

On 2019/7/31 15:11, Gao Xiang wrote:
> Hi Chao,
> 
> On 2019/7/31 15:05, Chao Yu wrote:
>> On 2019/7/29 14:51, Gao Xiang wrote:
>>> CONFIG_EROFS_FS_IO_MAX_RETRIES seems a runtime setting
>>> and users have no idea about the change in behaviour.
>>>
>>> Let's remove the setting currently and fold it into code,
>>> turn it into a module parameter if it's really needed.
>>>
>>> Suggested-by: David Sterba 
>>> Signed-off-by: Gao Xiang 
>>
>> It's fine to me, but I'd like to suggest to add this as a sys entry which 
>> can be
>> more flexible for user to change.
> 
> I think it can be added in the later version, the original view
> from David is that he had question how users using this option.
> 
> Maybe we can use the default value and leave it to users who
> really need to modify this value (real requirement).

I think we need to decide it in this version, otherwise it may face backward
compatibility issue if we change module argument to sys entry later.

Maybe just leave it as an fixed macro is fine, since there is actually no
requirement on this.

Thanks,

> 
> Thanks,
> Gao Xiang
> 
>>
>> Thanks
>>
> .
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 07/22] staging: erofs: remove redundant #include "internal.h"

[Chao Yu]

Hi Xiang,

On 2019/7/31 15:08, Gao Xiang wrote:
> Hi Chao,
> 
> On 2019/7/31 15:03, Chao Yu wrote:
>> On 2019/7/29 14:51, Gao Xiang wrote:
>>> Because #include "internal.h" is included in xattr.h
>>
>> I think it would be better to remove "internal.h" in xattr.h, and include 
>> them
>> both in .c file in where we need xattr definition.
> 
> It seems that all xattr related source files needing internal.h,
> and we need "EROFS_V(inode)", "struct erofs_sb_info", ... stuffs in xattr.h,
> which is defined in internal.h...

Since I checked f2fs', it looks it's okay to don't include internal.h for
xattr.h, if .c needs xattr.h, we can just include interanl.h and xattr.h in the
head of it, it's safe.

Thanks,

> 
> Thanks,
> Gao Xiang
> 
>>
>> Thanks,
>>
> .
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 22/22] staging: erofs: update Kconfig

[Chao Yu]

On 2019/7/29 14:51, Gao Xiang wrote:
> Keep in line with erofs-outofstaging patchset:
>  - turn on CONFIG_EROFS_FS_ZIP by default;
>  - turn on CONFIG_EROFS_FS_SECURITY by default suggested by David;
>  - update Kconfig description.
> 
> Signed-off-by: Gao Xiang 

Reviewed-by: Chao Yu 

Thanks,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 21/22] staging: erofs: update super.c

[Chao Yu]

On 2019/7/29 14:51, Gao Xiang wrote:
> Keep in line with erofs-outofstaging patchset:
> - "Chao Yu" is most commonly used in Linux community;

Either is okay to me, anyway, thanks for the correction. :)

BTW, I notice that .mailmap can map different email or name to single one on
git-shortlog or git-blame, so developer and user can find the correct people to
blame... I think we can change ours later.

> - quoted string split across lines.
> 
> Signed-off-by: Gao Xiang 

Reviewed-by: Chao Yu 

Thanks,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 20/22] staging: erofs: tidy up internal.h

[Chao Yu]

On 2019/7/29 14:51, Gao Xiang wrote:
> keep in line with erofs-outofstaging patchset:
>  - remove an extra #ifdef CONFIG_EROFS_FS_ZIP;
>  - add tags at the end of #endif acrossing several lines.
> 
> Signed-off-by: Gao Xiang 

Reviewed-by: Chao Yu 

Thanks,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 19/22] staging: erofs: tidy up utils.c

[Chao Yu]

On 2019/7/29 14:51, Gao Xiang wrote:
> keep in line with erofs-outofstaging patchset:
>  - Update comments in erofs_try_to_release_workgroup;
>  - code style cleanup.
> 
> Signed-off-by: Gao Xiang 

Reviewed-by: Chao Yu 

Thanks,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 18/22] staging: erofs: turn cache strategies into mount options

[Chao Yu]

On 2019/7/29 14:51, Gao Xiang wrote:
> Kill all kconfig cache strategies and turn them into mount options
> "cache_strategy={disable|readahead|readaround}".
> 
> As the first step, cached pages can still be usable after cache
> is disabled by remounting, and these pages will be fallen out
> over time, which can be refined in the later version if some
> requirement is needed. Update related document as well.
> 
> Suggested-by: Theodore Ts'o 
> Signed-off-by: Gao Xiang 

Reviewed-by: Chao Yu 

Thanks,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 17/22] staging: erofs: remove clusterbits in sbi

[Chao Yu]

On 2019/7/29 14:51, Gao Xiang wrote:
> clustersize can now be set on per-file basis
> rather than per-filesystem basis.
> 
> Signed-off-by: Gao Xiang 

Reviewed-by: Chao Yu 

Thanks,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 16/22] staging: erofs: tidy up decompression frontend

[Chao Yu]

On 2019/7/29 14:51, Gao Xiang wrote:
> Although this patch has an amount of changes, it is hard to
> separate into smaller patches.
> 
> Most changes are due to structure renaming for better understand
> and straightforward,
>  z_erofs_vle_workgroup to z_erofs_pcluster
>  since it represents a physical cluster;
>  z_erofs_vle_work to z_erofs_collection
>  since it represents a collection of logical pages;
>  z_erofs_vle_work_builder to z_erofs_collector
>  since it's used to fill z_erofs_{pcluster,collection}.
> 
> struct z_erofs_vle_work_finder has no extra use compared with
> struct z_erofs_collector, delete it.
> 
> FULL_LENGTH bit is integrated into .length of pcluster so that it
> can be updated with the corresponding length change in atomic.
> 
> Minor, add comments for better description.
> 
> Signed-off-by: Gao Xiang 

I hope I don't miss anything, since this is so huge cleanup...

Reviewed-by: Chao Yu 

Thanks,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 15/22] staging: erofs: remove redundant braces in inode.c

[Chao Yu]

On 2019/7/29 14:51, Gao Xiang wrote:
> Remove redundant braces in inode.c since
> these are all single statements.
> 
> Signed-off-by: Gao Xiang 

Reviewed-by: Chao Yu 

Thanks,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 14/22] staging: erofs: tidy up zpvec.h

[Chao Yu]

On 2019/7/29 14:51, Gao Xiang wrote:
> - use shorter function names:
>   z_erofs_pagevec_enqueue and z_erofs_pagevec_dequeue;
> - minor code cleanup.
> 
> In order to keep in line with erofs-outofstaging patchset.
> 
> Signed-off-by: Gao Xiang 

Reviewed-by: Chao Yu 

Thanks,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 13/22] staging: erofs: kill CONFIG_EROFS_FS_USE_VM_MAP_RAM

[Chao Yu]

On 2019/7/29 14:51, Gao Xiang wrote:
> Turn into a module parameter ("use_vmap") as it
> can be set at runtime.
> 
> Suggested-by: David Sterba 
> Signed-off-by: Gao Xiang 
> ---
>  drivers/staging/erofs/Kconfig|  8 --
>  drivers/staging/erofs/decompressor.c | 37 +++-
>  2 files changed, 20 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/staging/erofs/Kconfig b/drivers/staging/erofs/Kconfig
> index 747e9eebfaa5..788beebf3f7d 100644
> --- a/drivers/staging/erofs/Kconfig
> +++ b/drivers/staging/erofs/Kconfig
> @@ -63,14 +63,6 @@ config EROFS_FS_SECURITY
>  
> If you are not using a security module, say N.
>  
> -config EROFS_FS_USE_VM_MAP_RAM
> - bool "EROFS VM_MAP_RAM Support"
> - depends on EROFS_FS
> - help
> -   use vm_map_ram/vm_unmap_ram instead of vmap/vunmap.
> -
> -   If you don't know what these are, say N.
> -
>  config EROFS_FAULT_INJECTION
>   bool "EROFS fault injection facility"
>   depends on EROFS_FS
> diff --git a/drivers/staging/erofs/decompressor.c 
> b/drivers/staging/erofs/decompressor.c
> index 744c43a456e9..5352a50981cb 100644
> --- a/drivers/staging/erofs/decompressor.c
> +++ b/drivers/staging/erofs/decompressor.c
> @@ -7,6 +7,7 @@
>   * Created by Gao Xiang 
>   */
>  #include "compress.h"
> +#include 
>  #include 
>  
>  #ifndef LZ4_DISTANCE_MAX /* history window size */
> @@ -29,6 +30,10 @@ struct z_erofs_decompressor {
>   char *name;
>  };
>  
> +static bool use_vmap;
> +module_param(use_vmap, bool, 0444);
> +MODULE_PARM_DESC(use_vmap, "Use vmap() instead of vm_map_ram() (default 0)");

This should be documented in erofs.txt simply.

> +
>  static int lz4_prepare_destpages(struct z_erofs_decompress_req *rq,
>struct list_head *pagepool)
>  {
> @@ -219,29 +224,27 @@ static void copy_from_pcpubuf(struct page **out, const 
> char *dst,
>  
>  static void *erofs_vmap(struct page **pages, unsigned int count)
>  {
> -#ifdef CONFIG_EROFS_FS_USE_VM_MAP_RAM
> - int i = 0;
> -
> - while (1) {
> - void *addr = vm_map_ram(pages, count, -1, PAGE_KERNEL);
> - /* retry two more times (totally 3 times) */
> - if (addr || ++i >= 3)
> -     return addr;
> - vm_unmap_aliases();
> + if (!use_vmap) {

Minor thing.

if (use_vmap)
return vmap(pages, count, VM_MAP, PAGE_KERNEL);

while (1) {
}

return NULL;

Otherwise, it looks good to me.

Reviewed-by: Chao Yu 

Thanks,

> + int i = 0;
> +
> + while (1) {
> + void *addr = vm_map_ram(pages, count, -1, PAGE_KERNEL);
> + /* retry two more times (totally 3 times) */
> + if (addr || ++i >= 3)
> + return addr;
> + vm_unmap_aliases();
> + }
> + return NULL;
>   }
> - return NULL;
> -#else
>   return vmap(pages, count, VM_MAP, PAGE_KERNEL);
> -#endif
>  }
>  
>  static void erofs_vunmap(const void *mem, unsigned int count)
>  {
> -#ifdef CONFIG_EROFS_FS_USE_VM_MAP_RAM
> - vm_unmap_ram(mem, count);
> -#else
> - vunmap(mem);
> -#endif
> + if (!use_vmap)
> + vm_unmap_ram(mem, count);
> + else
> + vunmap(mem);
>  }
>  
>  static int decompress_generic(struct z_erofs_decompress_req *rq,
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 12/22] staging: erofs: refine erofs_allocpage()

[Chao Yu]

On 2019/7/29 14:51, Gao Xiang wrote:
> remove duplicated code in decompressor by introducing
> failable erofs_allocpage().
> 
> Signed-off-by: Gao Xiang 
> ---
>  drivers/staging/erofs/decompressor.c | 12 +++-
>  drivers/staging/erofs/internal.h |  2 +-
>  drivers/staging/erofs/utils.c|  5 +++--
>  drivers/staging/erofs/zdata.c|  2 +-
>  4 files changed, 8 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/staging/erofs/decompressor.c 
> b/drivers/staging/erofs/decompressor.c
> index ee5762351f80..744c43a456e9 100644
> --- a/drivers/staging/erofs/decompressor.c
> +++ b/drivers/staging/erofs/decompressor.c
> @@ -74,15 +74,9 @@ static int lz4_prepare_destpages(struct 
> z_erofs_decompress_req *rq,
>   victim = availables[--top];
>   get_page(victim);
>   } else {
> - if (!list_empty(pagepool)) {
> - victim = lru_to_page(pagepool);
> - list_del(>lru);
> - DBG_BUGON(page_ref_count(victim) != 1);
> - } else {
> - victim = alloc_pages(GFP_KERNEL, 0);
> - if (!victim)
> - return -ENOMEM;
> - }
> + victim = erofs_allocpage(pagepool, GFP_KERNEL, false);
> + if (unlikely(!victim))
> + return -ENOMEM;
>   victim->mapping = Z_EROFS_MAPPING_STAGING;
>   }
>   rq->out[i] = victim;
> diff --git a/drivers/staging/erofs/internal.h 
> b/drivers/staging/erofs/internal.h
> index b206a85776b4..e35c7d8f75d2 100644
> --- a/drivers/staging/erofs/internal.h
> +++ b/drivers/staging/erofs/internal.h
> @@ -517,7 +517,7 @@ int erofs_namei(struct inode *dir, struct qstr *name,
>  extern const struct file_operations erofs_dir_fops;
>  
>  /* utils.c / zdata.c */
> -struct page *erofs_allocpage(struct list_head *pool, gfp_t gfp);
> +struct page *erofs_allocpage(struct list_head *pool, gfp_t gfp, bool nofail);
>  
>  #if (EROFS_PCPUBUF_NR_PAGES > 0)
>  void *erofs_get_pcpubuf(unsigned int pagenr);
> diff --git a/drivers/staging/erofs/utils.c b/drivers/staging/erofs/utils.c
> index 0e86e44d60d0..260ea2970b4b 100644
> --- a/drivers/staging/erofs/utils.c
> +++ b/drivers/staging/erofs/utils.c
> @@ -9,15 +9,16 @@
>  #include "internal.h"
>  #include 
>  
> -struct page *erofs_allocpage(struct list_head *pool, gfp_t gfp)
> +struct page *erofs_allocpage(struct list_head *pool, gfp_t gfp, bool nofail)
>  {
>   struct page *page;
>  
>   if (!list_empty(pool)) {
>   page = lru_to_page(pool);
> + DBG_BUGON(page_ref_count(page) != 1);
>   list_del(>lru);
>   } else {
> - page = alloc_pages(gfp | __GFP_NOFAIL, 0);
> + page = alloc_pages(gfp | (nofail ? __GFP_NOFAIL : 0), 0);
>   }
>   return page;
>  }
> diff --git a/drivers/staging/erofs/zdata.c b/drivers/staging/erofs/zdata.c
> index bc478eebf509..02560b940558 100644
> --- a/drivers/staging/erofs/zdata.c
> +++ b/drivers/staging/erofs/zdata.c
> @@ -634,7 +634,7 @@ z_erofs_vle_work_iter_end(struct z_erofs_vle_work_builder 
> *builder)
>  static inline struct page *__stagingpage_alloc(struct list_head *pagepool,
>  gfp_t gfp)
>  {
> - struct page *page = erofs_allocpage(pagepool, gfp);
> + struct page *page = erofs_allocpage(pagepool, gfp, true);
>  
>   if (unlikely(!page))
>   return NULL;

Should remove it.

Otherwise, it looks good to me.

Reviewed-by: Chao Yu 

Thanks,

> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 11/22] staging: erofs: kill all failure handling in fill_super()

[Chao Yu]

On 2019/7/29 14:51, Gao Xiang wrote:
> .kill_sb() will do that instead in order to remove duplicated code.
> 
> Note that the initialzation of managed_cache is now moved
> after s_root is assigned since it's more preferred to iput()
> in .put_super() and all inodes should be evicted before
> the end of generic_shutdown_super(sb).
> 
> Suggested-by: Al Viro 
> Signed-off-by: Gao Xiang 
> ---
>  drivers/staging/erofs/super.c | 121 +++---
>  1 file changed, 53 insertions(+), 68 deletions(-)
> 
> diff --git a/drivers/staging/erofs/super.c b/drivers/staging/erofs/super.c
> index bfb6e1e09781..af5d87793e4d 100644
> --- a/drivers/staging/erofs/super.c
> +++ b/drivers/staging/erofs/super.c
> @@ -343,51 +343,52 @@ static const struct address_space_operations 
> managed_cache_aops = {
>   .invalidatepage = managed_cache_invalidatepage,
>  };
>  
> -static struct inode *erofs_init_managed_cache(struct super_block *sb)
> +static int erofs_init_managed_cache(struct super_block *sb)
>  {
> - struct inode *inode = new_inode(sb);
> + struct erofs_sb_info *const sbi = EROFS_SB(sb);
> + struct inode *const inode = new_inode(sb);
>  
>   if (unlikely(!inode))
> - return ERR_PTR(-ENOMEM);
> + return -ENOMEM;
>  
>   set_nlink(inode, 1);
>   inode->i_size = OFFSET_MAX;
>  
>   inode->i_mapping->a_ops = _cache_aops;
>   mapping_set_gfp_mask(inode->i_mapping,
> -  GFP_NOFS | __GFP_HIGHMEM |
> -  __GFP_MOVABLE |  __GFP_NOFAIL);
> - return inode;
> +  GFP_NOFS | __GFP_HIGHMEM | __GFP_MOVABLE);

It looks above change is not belong to this patch?

Otherwise, it looks good to me.

Reviewed-by: Chao Yu 

Thanks,

> + sbi->managed_cache = inode;
> + return 0;
>  }
> -
> +#else
> +static int erofs_init_managed_cache(struct super_block *sb) { return 0; }
>  #endif
>  
>  static int erofs_fill_super(struct super_block *sb, void *data, int silent)
>  {
>   struct inode *inode;
>   struct erofs_sb_info *sbi;
> - int err = -EINVAL;
> + int err;
>  
>   infoln("fill_super, device -> %s", sb->s_id);
>   infoln("options -> %s", (char *)data);
>  
> + sb->s_magic = EROFS_SUPER_MAGIC;
> +
>   if (unlikely(!sb_set_blocksize(sb, EROFS_BLKSIZ))) {
>   errln("failed to set erofs blksize");
> - goto err;
> + return -EINVAL;
>   }
>  
>   sbi = kzalloc(sizeof(*sbi), GFP_KERNEL);
> - if (unlikely(!sbi)) {
> - err = -ENOMEM;
> - goto err;
> - }
> - sb->s_fs_info = sbi;
> + if (unlikely(!sbi))
> + return -ENOMEM;
>  
> + sb->s_fs_info = sbi;
>   err = superblock_read(sb);
>   if (err)
> - goto err_sbread;
> + return err;
>  
> - sb->s_magic = EROFS_SUPER_MAGIC;
>   sb->s_flags |= SB_RDONLY | SB_NOATIME;
>   sb->s_maxbytes = MAX_LFS_FILESIZE;
>   sb->s_time_gran = 1;
> @@ -397,13 +398,12 @@ static int erofs_fill_super(struct super_block *sb, 
> void *data, int silent)
>  #ifdef CONFIG_EROFS_FS_XATTR
>   sb->s_xattr = erofs_xattr_handlers;
>  #endif
> -
>   /* set erofs default mount options */
>   default_options(sbi);
>  
>   err = parse_options(sb, data);
> - if (err)
> - goto err_parseopt;
> + if (unlikely(err))
> + return err;
>  
>   if (!silent)
>   infoln("root inode @ nid %llu", ROOT_NID(sbi));
> @@ -417,93 +417,78 @@ static int erofs_fill_super(struct super_block *sb, 
> void *data, int silent)
>   INIT_RADIX_TREE(>workstn_tree, GFP_ATOMIC);
>  #endif
>  
> -#ifdef EROFS_FS_HAS_MANAGED_CACHE
> - sbi->managed_cache = erofs_init_managed_cache(sb);
> - if (IS_ERR(sbi->managed_cache)) {
> - err = PTR_ERR(sbi->managed_cache);
> - goto err_init_managed_cache;
> - }
> -#endif
> -
>   /* get the root inode */
>   inode = erofs_iget(sb, ROOT_NID(sbi), true);
> - if (IS_ERR(inode)) {
> - err = PTR_ERR(inode);
> - goto err_iget;
> - }
> + if (IS_ERR(inode))
> + return PTR_ERR(inode);
>  
> - if (!S_ISDIR(inode->i_mode)) {
> + if (unlikely(!S_ISDIR(inode->i_mode))) {
>   errln("rootino(nid %llu) is not a directory(i_mode %o)",
> ROOT_NID(sbi), inode->i_mode);
> - err = -EINVAL;
>   

Re: [PATCH 10/22] staging: erofs: kill sbi->dev_name

[Chao Yu]

On 2019/7/29 14:51, Gao Xiang wrote:
> As Al said, "the only use of sbi->dev_name is debugging
> printks and all of those have sb->s_id available, with
> device name stored in there.  Which makes the whole
> thing bloody weird".
> 
> sbi->dev_name was used for our debugging use and it's
> better to just use s_id in community and delete
> the whole erofs_mount_private stuff.
> 
> Suggested-by: Al Viro 
> Signed-off-by: Gao Xiang 

Reviewed-by: Chao Yu 

Thanks,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 09/22] staging: erofs: clean up shrinker stuffs

[Chao Yu]

On 2019/7/29 14:51, Gao Xiang wrote:
> - rename erofs_register_super / erofs_unregister_super
>   to erofs_shrinker_register / erofs_shrinker_unregister;
> - fold the only erofs_shrink_workstation external call
>   to erofs_shrinker_unregister;
> - localize erofs_shrink_workstation;
> - localize erofs_shrinker_info by introducing
>   erofs_init_shrinker and erofs_exit_shrinker.
> 
> Signed-off-by: Gao Xiang 

Reviewed-by: Chao Yu 

Thanks,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 08/22] staging: erofs: kill CONFIG_EROFS_FS_IO_MAX_RETRIES

[Chao Yu]

On 2019/7/29 14:51, Gao Xiang wrote:
> CONFIG_EROFS_FS_IO_MAX_RETRIES seems a runtime setting
> and users have no idea about the change in behaviour.
> 
> Let's remove the setting currently and fold it into code,
> turn it into a module parameter if it's really needed.
> 
> Suggested-by: David Sterba 
> Signed-off-by: Gao Xiang 

It's fine to me, but I'd like to suggest to add this as a sys entry which can be
more flexible for user to change.

Thanks
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 07/22] staging: erofs: remove redundant #include "internal.h"

[Chao Yu]

On 2019/7/29 14:51, Gao Xiang wrote:
> Because #include "internal.h" is included in xattr.h

I think it would be better to remove "internal.h" in xattr.h, and include them
both in .c file in where we need xattr definition.

Thanks,

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 06/22] staging: erofs: clean up internal.h

[Chao Yu]

On 2019/7/29 14:51, Gao Xiang wrote:
> Tidy up relative order of variables / declarations in internal.h,
> and moving some local static functions out to other files.
> 
> No logic change.
> 
> Signed-off-by: Gao Xiang 

Reviewed-by: Chao Yu 

Thanks,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 05/22] staging: erofs: sunset erofs_workstn_{lock,unlock}

[Chao Yu]

On 2019/7/29 14:51, Gao Xiang wrote:
> It was used for Linux backward compatibility, and no use
> for upstream kernel.
> 
> Signed-off-by: Gao Xiang 

Reviewed-by: Chao Yu 

Thanks,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 04/22] staging: erofs: keep up erofs_fs.h with erofs-outofstaging patchset

[Chao Yu]

On 2019/7/29 14:51, Gao Xiang wrote:
> The main change is to reserve all checksums except for superblock,
> since it's more useful to do block-based verity for read-only fs.
> 
> Some comments change as well, which is minor.
> 
> Signed-off-by: Gao Xiang 

Reviewed-by: Chao Yu 

Thanks,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 03/22] staging: erofs: fix dummy functions erofs_{get, list}xattr

[Chao Yu]

On 2019/7/29 14:51, Gao Xiang wrote:
> dummy functions erofs_{get,list}xattr should be inlined
> without xattr enabled.
> 
> Signed-off-by: Yue Hu 
> [ Gao Xiang : this patch was "staging: erofs: remove needless
>   dummy functions of erofs_{get,list}xattr. "]
> Signed-off-by: Gao Xiang 

Reviewed-by: Chao Yu 

Thanks,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 02/22] staging: erofs: rename source files for better understanding

[Chao Yu]

On 2019/7/29 14:51, Gao Xiang wrote:
> Keep in line with erofs-outofstaging patchset as well, see
> https://lore.kernel.org/linux-fsdevel/20190725095658.155779-1-gaoxian...@huawei.com/
> 
> Signed-off-by: Gao Xiang 

Reviewed-by: Chao Yu 

Thanks,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 01/22] staging: erofs: update source file headers

[Chao Yu]

On 2019/7/29 14:51, Gao Xiang wrote:
> - Use the correct style for all SPDX License Identifiers;
> - Get rid of the unnecessary license boilerplate;
> - Use "GPL-2.0-only" instead of "GPL-2.0" suggested-by Stephen.
> 
> Signed-off-by: Gao Xiang 

Reviewed-by: Chao Yu 

Thanks,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] v3: staging: erofs: fix typo

[Chao Yu]

On 2019/7/18 4:11, Karen Palacio wrote:
> Fix typo in Kconfig
> 
> Signed-off-by: Karen Palacio 

Reviewed-by: Chao Yu 

Thanks,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2] staging: erofs: avoid opened loop codes

[Chao Yu]

Use __GFP_NOFAIL to avoid opened loop codes in z_erofs_vle_unzip().

Signed-off-by: Chao Yu 
---
v2:
- change variable name from 'flags' to 'gfp_flags' to avoid common
name.
 drivers/staging/erofs/unzip_vle.c | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/erofs/unzip_vle.c 
b/drivers/staging/erofs/unzip_vle.c
index f0dab81ff816..56c009cf611e 100644
--- a/drivers/staging/erofs/unzip_vle.c
+++ b/drivers/staging/erofs/unzip_vle.c
@@ -921,18 +921,18 @@ static int z_erofs_vle_unzip(struct super_block *sb,
 mutex_trylock(_pagemap_global_lock))
pages = z_pagemap_global;
else {
-repeat:
+   gfp_t gfp_flags = GFP_KERNEL;
+
+   if (nr_pages > Z_EROFS_VLE_VMAP_GLOBAL_PAGES)
+   gfp_flags |= __GFP_NOFAIL;
+
pages = kvmalloc_array(nr_pages, sizeof(struct page *),
-  GFP_KERNEL);
+  gfp_flags);
 
/* fallback to global pagemap for the lowmem scenario */
if (unlikely(!pages)) {
-   if (nr_pages > Z_EROFS_VLE_VMAP_GLOBAL_PAGES)
-   goto repeat;
-   else {
-   mutex_lock(_pagemap_global_lock);
-   pages = z_pagemap_global;
-   }
+   mutex_lock(_pagemap_global_lock);
+   pages = z_pagemap_global;
}
}
 
-- 
2.18.0.rc1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v3] staging: erofs: support bmap

[Chao Yu]

Add erofs_bmap() to support FIBMAP ioctl on flatmode inode.

Reviewed-by: Gao Xiang 
Signed-off-by: Chao Yu 
---
v3:
- use erofs_blk_t instead of unsigned int.
- simply judgment condition.
 drivers/staging/erofs/data.c | 33 +
 1 file changed, 33 insertions(+)

diff --git a/drivers/staging/erofs/data.c b/drivers/staging/erofs/data.c
index cc31c3e5984c..f73e4720cd3e 100644
--- a/drivers/staging/erofs/data.c
+++ b/drivers/staging/erofs/data.c
@@ -392,9 +392,42 @@ static int erofs_raw_access_readpages(struct file *filp,
return 0;
 }
 
+static int erofs_get_block(struct inode *inode, sector_t iblock,
+  struct buffer_head *bh, int create)
+{
+   struct erofs_map_blocks map = {
+   .m_la = iblock << 9,
+   };
+   int err;
+
+   err = erofs_map_blocks(inode, , EROFS_GET_BLOCKS_RAW);
+   if (err)
+   return err;
+
+   if (map.m_flags & EROFS_MAP_MAPPED)
+   bh->b_blocknr = erofs_blknr(map.m_pa);
+
+   return err;
+}
+
+static sector_t erofs_bmap(struct address_space *mapping, sector_t block)
+{
+   struct inode *inode = mapping->host;
+
+   if (is_inode_flat_inline(inode)) {
+   erofs_blk_t blks = i_size_read(inode) >> LOG_BLOCK_SIZE;
+
+   if (block >> LOG_SECTORS_PER_BLOCK >= blks)
+   return 0;
+   }
+
+   return generic_block_bmap(mapping, block, erofs_get_block);
+}
+
 /* for uncompressed (aligned) files and raw access for other files */
 const struct address_space_operations erofs_raw_access_aops = {
.readpage = erofs_raw_access_readpage,
.readpages = erofs_raw_access_readpages,
+   .bmap = erofs_bmap,
 };
 
-- 
2.18.0.rc1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: erofs: avoid opened loop codes

[Chao Yu]

Hi Xiang,

On 2019/7/16 17:12, Gao Xiang wrote:
> Hi Chao,
> 
> On 2019/7/16 16:52, Chao Yu wrote:
>> Use __GFP_NOFAIL to avoid opened loop codes in z_erofs_vle_unzip().
>>
>> Signed-off-by: Chao Yu 
>> ---
>>  drivers/staging/erofs/unzip_vle.c | 17 -
>>  1 file changed, 8 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/staging/erofs/unzip_vle.c 
>> b/drivers/staging/erofs/unzip_vle.c
>> index f0dab81ff816..3a0dbcb8cc9f 100644
>> --- a/drivers/staging/erofs/unzip_vle.c
>> +++ b/drivers/staging/erofs/unzip_vle.c
>> @@ -921,18 +921,17 @@ static int z_erofs_vle_unzip(struct super_block *sb,
>>   mutex_trylock(_pagemap_global_lock))
>>  pages = z_pagemap_global;
>>  else {
>> -repeat:
>> -pages = kvmalloc_array(nr_pages, sizeof(struct page *),
>> -   GFP_KERNEL);
>> +gfp_t flags = GFP_KERNEL;
>> +
>> +if (nr_pages > Z_EROFS_VLE_VMAP_GLOBAL_PAGES)
>> +flags |= __GFP_NOFAIL;
>> +
>> +pages = kvmalloc_array(nr_pages, sizeof(struct page *), flags);
> 
> How about omitting variable `flags' since it's only used once, or
> rename it since `flags' is too general?  some thoughts?

That's minor thing, if you do care about this, I guess 'gfp_flags' maybe?

> 
> BTW, This piece of code has been changed in
> "[PATCH v2 00/24] erofs: promote erofs from staging", I will sync the code
> after some guys takes a look at v2

We have enough time to merge modified staging codes to generic fs one till next
merge window, so don't worry, you can do the merge in batch.

Thanks,

> 
> Thanks,
> Gao Xiang
> 
>>  
>>  /* fallback to global pagemap for the lowmem scenario */
>>  if (unlikely(!pages)) {
>> -if (nr_pages > Z_EROFS_VLE_VMAP_GLOBAL_PAGES)
>> -goto repeat;
>> -else {
>> -mutex_lock(_pagemap_global_lock);
>> -pages = z_pagemap_global;
>> -}
>> +mutex_lock(_pagemap_global_lock);
>> +pages = z_pagemap_global;
>>  }
>>  }
>>  
>>
> .
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] staging: erofs: support bmap

[Chao Yu]

Hi Xiang,

On 2019/7/16 17:01, Gao Xiang wrote:
> Hi Chao,
> 
> Cc lkml mailing list?

Oh, no problem, let me update my default cc list.

> 
> On 2019/7/16 16:46, Chao Yu wrote:
>> Add erofs_bmap() to support FIBMAP ioctl on flatmode inode.
>>
>> Signed-off-by: Chao Yu 
>> ---
>> v2:
>> - support mapping normal blocks for inline inode suggested by Xiang
>> - rebase to linux-next
>>  drivers/staging/erofs/data.c | 33 +
>>  1 file changed, 33 insertions(+)
>>
>> diff --git a/drivers/staging/erofs/data.c b/drivers/staging/erofs/data.c
>> index cc31c3e5984c..5c7e3b9cff6a 100644
>> --- a/drivers/staging/erofs/data.c
>> +++ b/drivers/staging/erofs/data.c
>> @@ -392,9 +392,42 @@ static int erofs_raw_access_readpages(struct file *filp,
>>  return 0;
>>  }
>>  
>> +static int erofs_get_block(struct inode *inode, sector_t iblock,
>> +   struct buffer_head *bh, int create)
>> +{
>> +struct erofs_map_blocks map = {
>> +.m_la = iblock << 9,
>> +};
>> +int err;
>> +
>> +err = erofs_map_blocks(inode, , EROFS_GET_BLOCKS_RAW);
>> +if (err)
>> +return err;
>> +
>> +if (map.m_flags & EROFS_MAP_MAPPED)
>> +bh->b_blocknr = erofs_blknr(map.m_pa);
>> +
>> +return err;
>> +}
>> +
>> +static sector_t erofs_bmap(struct address_space *mapping, sector_t block)
>> +{
>> +struct inode *inode = mapping->host;
>> +
>> +if (is_inode_flat_inline(inode)) {
>> +unsigned int blks = i_size_read(inode) >> LOG_BLOCK_SIZE;
> 
> maybe we could use erofs_blk_t ?

Okay,

> 
>> +
>> +if (!blks || block >> LOG_SECTORS_PER_BLOCK >= blks)
> 
> Could the above line be simplified to block >> LOG_SECTORS_PER_BLOCK >= blks?

That's right, original one was trying to consider readability and avoid
calculation of following statement when blks equals to zero.

Anyway, either is okay to me.

> 
> Other parts looks good to me,
> Reviewed-by: Gao Xiang 
> 
> However, I think resend a new patch is better.

Yeah, let me send v3.

Thanks,

> 
> Thanks,
> Gao Xiang
> 
>> +return 0;
>> +}
>> +
>> +return generic_block_bmap(mapping, block, erofs_get_block);
>> +}
>> +
>>  /* for uncompressed (aligned) files and raw access for other files */
>>  const struct address_space_operations erofs_raw_access_aops = {
>>  .readpage = erofs_raw_access_readpage,
>>  .readpages = erofs_raw_access_readpages,
>> +.bmap = erofs_bmap,
>>  };
>>  
>>
> .
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: erofs: support bmap

[Chao Yu]

On 2019/7/16 15:37, Gao Xiang wrote:
> 
> 
> On 2019/7/16 15:35, Chao Yu wrote:
>> Hi Xiang,
>>
>> On 2019/7/16 15:19, Gao Xiang wrote:
>>> Hi Chao,
>>>
>>> On 2019/7/16 15:05, Chao Yu wrote:
>>>> Add erofs_bmap() to support FIBMAP ioctl on flatmode inode.
>>>>
>>>> Signed-off-by: Chao Yu 
>>>> ---
>>>>  drivers/staging/erofs/data.c | 29 +
>>>>  1 file changed, 29 insertions(+)
>>>>
>>>> diff --git a/drivers/staging/erofs/data.c b/drivers/staging/erofs/data.c
>>>> index fbce71403d87..03da57f04347 100644
>>>> --- a/drivers/staging/erofs/data.c
>>>> +++ b/drivers/staging/erofs/data.c
>>>> @@ -308,9 +308,38 @@ static int erofs_raw_access_readpages(struct file 
>>>> *filp,
>>>>return iomap_readpages(mapping, pages, nr_pages, _iomap_ops);
>>>>  }
>>>>  
>>>> +static int erofs_get_block(struct inode *inode, sector_t iblock,
>>>> + struct buffer_head *bh, int create)
>>>> +{
>>>> +  struct erofs_map_blocks map = {
>>>> +  .m_la = iblock << 9,
>>>> +  };
>>>> +  int err;
>>>> +
>>>> +  err = erofs_map_blocks(inode, , EROFS_GET_BLOCKS_RAW);
>>>> +  if (err)
>>>> +  return err;
>>>> +
>>>> +  if (map.m_flags & EROFS_MAP_MAPPED)
>>>> +  bh->b_blocknr = erofs_blknr(map.m_pa);
>>>> +
>>>> +  return err;
>>>> +}
>>>> +
>>>> +static sector_t erofs_bmap(struct address_space *mapping, sector_t block)
>>>> +{
>>>> +  struct inode *inode = mapping->host;
>>>> +
>>>> +  if (is_inode_flat_inline(inode))
>>>> +  return 0;
>>>
>>> could we support flat_inline as well? some difficulty on this?
>>
>> I think we can support partial of this on inline inode, e.g.:
>>
>> [0, 4k] mapped
>> [4k, 5k] inline
>>
>> We can find and return the physical block address for first block, but for 
>> last
>> block locating in metadata, we should just return 0 (0 indicate bmap has 
>> failed
>> to get mapped block address).
> 
> I think that is fine (in the long term, FIEMAP is preferred of course.)

Right, I can take a look at fiemap() implementation.

Thanks,

> 
> Thanks,
> Gao Xiang
> .
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] staging: erofs: avoid opened loop codes

[Chao Yu]

Use __GFP_NOFAIL to avoid opened loop codes in z_erofs_vle_unzip().

Signed-off-by: Chao Yu 
---
 drivers/staging/erofs/unzip_vle.c | 17 -
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/erofs/unzip_vle.c 
b/drivers/staging/erofs/unzip_vle.c
index f0dab81ff816..3a0dbcb8cc9f 100644
--- a/drivers/staging/erofs/unzip_vle.c
+++ b/drivers/staging/erofs/unzip_vle.c
@@ -921,18 +921,17 @@ static int z_erofs_vle_unzip(struct super_block *sb,
 mutex_trylock(_pagemap_global_lock))
pages = z_pagemap_global;
else {
-repeat:
-   pages = kvmalloc_array(nr_pages, sizeof(struct page *),
-  GFP_KERNEL);
+   gfp_t flags = GFP_KERNEL;
+
+   if (nr_pages > Z_EROFS_VLE_VMAP_GLOBAL_PAGES)
+   flags |= __GFP_NOFAIL;
+
+   pages = kvmalloc_array(nr_pages, sizeof(struct page *), flags);
 
/* fallback to global pagemap for the lowmem scenario */
if (unlikely(!pages)) {
-   if (nr_pages > Z_EROFS_VLE_VMAP_GLOBAL_PAGES)
-   goto repeat;
-   else {
-   mutex_lock(_pagemap_global_lock);
-   pages = z_pagemap_global;
-   }
+   mutex_lock(_pagemap_global_lock);
+   pages = z_pagemap_global;
}
}
 
-- 
2.18.0.rc1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2] staging: erofs: support bmap

[Chao Yu]

Add erofs_bmap() to support FIBMAP ioctl on flatmode inode.

Signed-off-by: Chao Yu 
---
v2:
- support mapping normal blocks for inline inode suggested by Xiang
- rebase to linux-next
 drivers/staging/erofs/data.c | 33 +
 1 file changed, 33 insertions(+)

diff --git a/drivers/staging/erofs/data.c b/drivers/staging/erofs/data.c
index cc31c3e5984c..5c7e3b9cff6a 100644
--- a/drivers/staging/erofs/data.c
+++ b/drivers/staging/erofs/data.c
@@ -392,9 +392,42 @@ static int erofs_raw_access_readpages(struct file *filp,
return 0;
 }
 
+static int erofs_get_block(struct inode *inode, sector_t iblock,
+  struct buffer_head *bh, int create)
+{
+   struct erofs_map_blocks map = {
+   .m_la = iblock << 9,
+   };
+   int err;
+
+   err = erofs_map_blocks(inode, , EROFS_GET_BLOCKS_RAW);
+   if (err)
+   return err;
+
+   if (map.m_flags & EROFS_MAP_MAPPED)
+   bh->b_blocknr = erofs_blknr(map.m_pa);
+
+   return err;
+}
+
+static sector_t erofs_bmap(struct address_space *mapping, sector_t block)
+{
+   struct inode *inode = mapping->host;
+
+   if (is_inode_flat_inline(inode)) {
+   unsigned int blks = i_size_read(inode) >> LOG_BLOCK_SIZE;
+
+   if (!blks || block >> LOG_SECTORS_PER_BLOCK >= blks)
+   return 0;
+   }
+
+   return generic_block_bmap(mapping, block, erofs_get_block);
+}
+
 /* for uncompressed (aligned) files and raw access for other files */
 const struct address_space_operations erofs_raw_access_aops = {
.readpage = erofs_raw_access_readpage,
.readpages = erofs_raw_access_readpages,
+   .bmap = erofs_bmap,
 };
 
-- 
2.18.0.rc1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: erofs: support bmap

[Chao Yu]

Hi Xiang,

On 2019/7/16 15:19, Gao Xiang wrote:
> Hi Chao,
> 
> On 2019/7/16 15:05, Chao Yu wrote:
>> Add erofs_bmap() to support FIBMAP ioctl on flatmode inode.
>>
>> Signed-off-by: Chao Yu 
>> ---
>>  drivers/staging/erofs/data.c | 29 +
>>  1 file changed, 29 insertions(+)
>>
>> diff --git a/drivers/staging/erofs/data.c b/drivers/staging/erofs/data.c
>> index fbce71403d87..03da57f04347 100644
>> --- a/drivers/staging/erofs/data.c
>> +++ b/drivers/staging/erofs/data.c
>> @@ -308,9 +308,38 @@ static int erofs_raw_access_readpages(struct file *filp,
>>  return iomap_readpages(mapping, pages, nr_pages, _iomap_ops);
>>  }
>>  
>> +static int erofs_get_block(struct inode *inode, sector_t iblock,
>> +   struct buffer_head *bh, int create)
>> +{
>> +struct erofs_map_blocks map = {
>> +.m_la = iblock << 9,
>> +};
>> +int err;
>> +
>> +err = erofs_map_blocks(inode, , EROFS_GET_BLOCKS_RAW);
>> +if (err)
>> +return err;
>> +
>> +if (map.m_flags & EROFS_MAP_MAPPED)
>> +bh->b_blocknr = erofs_blknr(map.m_pa);
>> +
>> +return err;
>> +}
>> +
>> +static sector_t erofs_bmap(struct address_space *mapping, sector_t block)
>> +{
>> +struct inode *inode = mapping->host;
>> +
>> +if (is_inode_flat_inline(inode))
>> +return 0;
> 
> could we support flat_inline as well? some difficulty on this?

I think we can support partial of this on inline inode, e.g.:

[0, 4k] mapped
[4k, 5k] inline

We can find and return the physical block address for first block, but for last
block locating in metadata, we should just return 0 (0 indicate bmap has failed
to get mapped block address).

Thanks,

> (I have no idea return 0 is suitable for us since we have tail-end packing...)
> 
> Thanks,
> Gao Xiang
> 
>> +
>> +return generic_block_bmap(mapping, block, erofs_get_block);
>> +}
>> +
>>  /* for uncompressed (aligned) files and raw access for other files */
>>  const struct address_space_operations erofs_raw_access_aops = {
>>  .readpage = erofs_raw_access_readpage,
>>  .readpages = erofs_raw_access_readpages,
>> +.bmap = erofs_bmap,
>>  };
>>  
>>
> .
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] staging: erofs: support bmap

[Chao Yu]

Add erofs_bmap() to support FIBMAP ioctl on flatmode inode.

Signed-off-by: Chao Yu 
---
 drivers/staging/erofs/data.c | 29 +
 1 file changed, 29 insertions(+)

diff --git a/drivers/staging/erofs/data.c b/drivers/staging/erofs/data.c
index fbce71403d87..03da57f04347 100644
--- a/drivers/staging/erofs/data.c
+++ b/drivers/staging/erofs/data.c
@@ -308,9 +308,38 @@ static int erofs_raw_access_readpages(struct file *filp,
return iomap_readpages(mapping, pages, nr_pages, _iomap_ops);
 }
 
+static int erofs_get_block(struct inode *inode, sector_t iblock,
+  struct buffer_head *bh, int create)
+{
+   struct erofs_map_blocks map = {
+   .m_la = iblock << 9,
+   };
+   int err;
+
+   err = erofs_map_blocks(inode, , EROFS_GET_BLOCKS_RAW);
+   if (err)
+   return err;
+
+   if (map.m_flags & EROFS_MAP_MAPPED)
+   bh->b_blocknr = erofs_blknr(map.m_pa);
+
+   return err;
+}
+
+static sector_t erofs_bmap(struct address_space *mapping, sector_t block)
+{
+   struct inode *inode = mapping->host;
+
+   if (is_inode_flat_inline(inode))
+   return 0;
+
+   return generic_block_bmap(mapping, block, erofs_get_block);
+}
+
 /* for uncompressed (aligned) files and raw access for other files */
 const struct address_space_operations erofs_raw_access_aops = {
.readpage = erofs_raw_access_readpage,
.readpages = erofs_raw_access_readpages,
+   .bmap = erofs_bmap,
 };
 
-- 
2.18.0.rc1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: erofs: Remove function erofs_kill_sb()

[Chao Yu]

On 2019/7/12 15:12, Nishka Dasgupta wrote:
> Remove function erofs_kill_sb as all it does is call kill_block_super.
> Modify references to the former to point to the latter.
> Issue found with Coccinelle.
> 
> Signed-off-by: Nishka Dasgupta 

Reviewed-by: Chao Yu 

Thanks,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: erofs: fix LZ4 limited bounced page mis-reuse

[Chao Yu]

Hi xiang,

On 2019/7/3 14:06, Gao Xiang wrote:
> Hi Chao,
> 
> On 2019/7/3 10:09, Gao Xiang wrote:
>>
>>
>> On 2019/7/3 9:50, Chao Yu wrote:
>>> On 2019/7/1 2:58, Gao Xiang wrote:
>>>> From: Gao Xiang 
>>>>
>>>> Like all lz77-based algrithms, lz4 has a dynamically populated
>>>> ("sliding window") dictionary and the maximum lookback distance
>>>> is 65535. Therefore the number of bounced pages could be limited
>>>> by erofs based on this property.
>>>>
>>>> However, just now we observed some lz4 sequences in the extreme
>>>> case cannot be decompressed correctly after this feature is enabled,
>>>> the root causes after analysis are clear as follows:
>>>> 1) max bounced pages should be 17 rather than 16 pages;
>>>> 2) considering the following case, the broken implementation
>>>>could reuse unsafely in advance (in other words, reuse it
>>>>less than a safe distance),
>>>>0 1 2 ... 16 17 18 ... 33 34
>>>>b p  b b
>>>>note that the bounce page that we are concerned was allocated
>>>>at 0, and it reused at 18 since page 17 exists, but it mis-reused
>>>>at 34 in advance again, which causes decompress failure.
>>>>
>>>> This patch resolves the issue by introducing a bitmap to mark
>>>> whether the page in the same position of last round is a bounced
>>>> page or not, and a micro stack data structure to store all
>>>> available bounced pages.
>>>>
>>>> Fixes: 7fc45dbc938a ("staging: erofs: introduce generic decompression 
>>>> backend")
>>>> Signed-off-by: Gao Xiang 
>>>> ---
>>>>  drivers/staging/erofs/decompressor.c | 50 
>>>>  1 file changed, 28 insertions(+), 22 deletions(-)
>>>>
>>>> diff --git a/drivers/staging/erofs/decompressor.c 
>>>> b/drivers/staging/erofs/decompressor.c
>>>> index 80f1f39719ba..1fb0abb98dff 100644
>>>> --- a/drivers/staging/erofs/decompressor.c
>>>> +++ b/drivers/staging/erofs/decompressor.c
>>>> @@ -13,7 +13,7 @@
>>>>  #define LZ4_DISTANCE_MAX 65535/* set to maximum value by default */
>>>>  #endif
>>>>  
>>>> -#define LZ4_MAX_DISTANCE_PAGESDIV_ROUND_UP(LZ4_DISTANCE_MAX, 
>>>> PAGE_SIZE)
>>>> +#define LZ4_MAX_DISTANCE_PAGES(DIV_ROUND_UP(LZ4_DISTANCE_MAX, 
>>>> PAGE_SIZE) + 1)
>>>>  #ifndef LZ4_DECOMPRESS_INPLACE_MARGIN
>>>>  #define LZ4_DECOMPRESS_INPLACE_MARGIN(srcsize)  (((srcsize) >> 8) + 32)
>>>>  #endif
>>>> @@ -35,19 +35,28 @@ static int lz4_prepare_destpages(struct 
>>>> z_erofs_decompress_req *rq,
>>>>const unsigned int nr =
>>>>PAGE_ALIGN(rq->pageofs_out + rq->outputsize) >> PAGE_SHIFT;
>>>>struct page *availables[LZ4_MAX_DISTANCE_PAGES] = { NULL };
>>>> -  unsigned long unused[DIV_ROUND_UP(LZ4_MAX_DISTANCE_PAGES,
>>>> -BITS_PER_LONG)] = { 0 };
>>>> +  unsigned long bounced[DIV_ROUND_UP(LZ4_MAX_DISTANCE_PAGES,
>>>> + BITS_PER_LONG)] = { 0 };
>>>>void *kaddr = NULL;
>>>> -  unsigned int i, j, k;
>>>> +  unsigned int i, j, top;
>>>>  
>>>> -  for (i = 0; i < nr; ++i) {
>>>> +  top = 0;
>>>> +  for (i = j = 0; i < nr; ++i, ++j) {
>>>>struct page *const page = rq->out[i];
>>>> +  struct page *victim;
>>>>  
>>>> -  j = i & (LZ4_MAX_DISTANCE_PAGES - 1);
>>>> -  if (availables[j])
>>>> -  __set_bit(j, unused);
>>>> +  if (j >= LZ4_MAX_DISTANCE_PAGES)
>>>> +  j = 0;
>>>> +
>>>> +  /* 'valid' bounced can only be tested after a complete round */
>>>> +  if (test_bit(j, bounced)) {
>>>> +  DBG_BUGON(i < LZ4_MAX_DISTANCE_PAGES);
>>>> +  DBG_BUGON(top >= LZ4_MAX_DISTANCE_PAGES);
>>>> +  availables[top++] = rq->out[i - LZ4_MAX_DISTANCE_PAGES];
>>>
>>> Maybe we can change 'i - LZ4_MAX_DISTANCE_PAGES' to 'j' directly for better
>>> readability.
>>
>> OK, I think they are equivalent as well, will change for readabil

Re: [PATCH] staging: erofs: fix LZ4 limited bounced page mis-reuse

[Chao Yu]

On 2019/7/1 2:58, Gao Xiang wrote:
> From: Gao Xiang 
> 
> Like all lz77-based algrithms, lz4 has a dynamically populated
> ("sliding window") dictionary and the maximum lookback distance
> is 65535. Therefore the number of bounced pages could be limited
> by erofs based on this property.
> 
> However, just now we observed some lz4 sequences in the extreme
> case cannot be decompressed correctly after this feature is enabled,
> the root causes after analysis are clear as follows:
> 1) max bounced pages should be 17 rather than 16 pages;
> 2) considering the following case, the broken implementation
>could reuse unsafely in advance (in other words, reuse it
>less than a safe distance),
>0 1 2 ... 16 17 18 ... 33 34
>b p  b b
>note that the bounce page that we are concerned was allocated
>at 0, and it reused at 18 since page 17 exists, but it mis-reused
>at 34 in advance again, which causes decompress failure.
> 
> This patch resolves the issue by introducing a bitmap to mark
> whether the page in the same position of last round is a bounced
> page or not, and a micro stack data structure to store all
> available bounced pages.
> 
> Fixes: 7fc45dbc938a ("staging: erofs: introduce generic decompression 
> backend")
> Signed-off-by: Gao Xiang 
> ---
>  drivers/staging/erofs/decompressor.c | 50 
>  1 file changed, 28 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/staging/erofs/decompressor.c 
> b/drivers/staging/erofs/decompressor.c
> index 80f1f39719ba..1fb0abb98dff 100644
> --- a/drivers/staging/erofs/decompressor.c
> +++ b/drivers/staging/erofs/decompressor.c
> @@ -13,7 +13,7 @@
>  #define LZ4_DISTANCE_MAX 65535   /* set to maximum value by default */
>  #endif
>  
> -#define LZ4_MAX_DISTANCE_PAGES   DIV_ROUND_UP(LZ4_DISTANCE_MAX, 
> PAGE_SIZE)
> +#define LZ4_MAX_DISTANCE_PAGES   (DIV_ROUND_UP(LZ4_DISTANCE_MAX, 
> PAGE_SIZE) + 1)
>  #ifndef LZ4_DECOMPRESS_INPLACE_MARGIN
>  #define LZ4_DECOMPRESS_INPLACE_MARGIN(srcsize)  (((srcsize) >> 8) + 32)
>  #endif
> @@ -35,19 +35,28 @@ static int lz4_prepare_destpages(struct 
> z_erofs_decompress_req *rq,
>   const unsigned int nr =
>   PAGE_ALIGN(rq->pageofs_out + rq->outputsize) >> PAGE_SHIFT;
>   struct page *availables[LZ4_MAX_DISTANCE_PAGES] = { NULL };
> - unsigned long unused[DIV_ROUND_UP(LZ4_MAX_DISTANCE_PAGES,
> -   BITS_PER_LONG)] = { 0 };
> + unsigned long bounced[DIV_ROUND_UP(LZ4_MAX_DISTANCE_PAGES,
> +BITS_PER_LONG)] = { 0 };
>   void *kaddr = NULL;
> - unsigned int i, j, k;
> + unsigned int i, j, top;
>  
> - for (i = 0; i < nr; ++i) {
> + top = 0;
> + for (i = j = 0; i < nr; ++i, ++j) {
>   struct page *const page = rq->out[i];
> + struct page *victim;
>  
> - j = i & (LZ4_MAX_DISTANCE_PAGES - 1);
> - if (availables[j])
> - __set_bit(j, unused);
> + if (j >= LZ4_MAX_DISTANCE_PAGES)
> + j = 0;
> +
> + /* 'valid' bounced can only be tested after a complete round */
> + if (test_bit(j, bounced)) {
> + DBG_BUGON(i < LZ4_MAX_DISTANCE_PAGES);
> + DBG_BUGON(top >= LZ4_MAX_DISTANCE_PAGES);
> + availables[top++] = rq->out[i - LZ4_MAX_DISTANCE_PAGES];

Maybe we can change 'i - LZ4_MAX_DISTANCE_PAGES' to 'j' directly for better
readability.

Otherwise, it looks good to me.

Reviewed-by: Chao Yu 

Thanks,

> + }
>  
>   if (page) {
> + __clear_bit(j, bounced);
>   if (kaddr) {
>   if (kaddr + PAGE_SIZE == page_address(page))
>   kaddr += PAGE_SIZE;
> @@ -59,27 +68,24 @@ static int lz4_prepare_destpages(struct 
> z_erofs_decompress_req *rq,
>   continue;
>   }
>   kaddr = NULL;
> + __set_bit(j, bounced);
>  
> - k = find_first_bit(unused, LZ4_MAX_DISTANCE_PAGES);
> - if (k < LZ4_MAX_DISTANCE_PAGES) {
> - j = k;
> - get_page(availables[j]);
> + if (top) {
> + victim = availables[--top];
> + get_page(victim);
>   } else {
> - DBG_BUGON(availables[j]);
> -
>   if (!list_empty(pagepool)) {
> - availa

Re: [PATCH] staging: erofs: don't check special inode layout

[Chao Yu]

On 2019/6/28 12:19, Yue Hu wrote:
> On Fri, 28 Jun 2019 11:50:21 +0800
> Gao Xiang  wrote:
> 
>> Hi Yue,
>>
>> On 2019/6/28 11:42, Yue Hu wrote:
>>> From: Yue Hu 
>>>
>>> Currently, we will check if inode layout is compression or inline if
>>> the inode is special in fill_inode(). Also set ->i_mapping->a_ops for
>>> it. That is pointless since the both modes won't be set for special
>>> inode when creating EROFS filesystem image. So, let's avoid it.
>>>
>>> Signed-off-by: Yue Hu   
>>
>> Have you test this patch with some actual image with legacy mkfs since
>> new mkfs framework have not supported special inode...
> 
> Hi Xiang,
> 
> I'm studying the testing :)
> 
> However, already check the code handling for special inode in leagcy mkfs as 
> below:
> 
> ```c
> break;
> case EROFS_FT_BLKDEV:
> case EROFS_FT_CHRDEV:
> case EROFS_FT_FIFO:
> case EROFS_FT_SOCK:
> mkfs_rank_inode(d);
> break;
> 
> default:
> erofs_err("inode[%s] file_type error =%d",
>   d->i_fullpath,
> ```
> 
> No special inode layout operations, so this change should be fine.
> 
> Thx.
> 
>>
>> I think that is fine in priciple, however, in case to introduce some 
>> potential
>> issues, I will test this patch later. I will give a Reviewed-by tag after I 
>> tested
>> this patch.

This patch looks good to me, if this won't fail any tests from Xiang, you can 
add:

Reviewed-by: Chao Yu 

Thanks,

> 
> Thanks.
> 
>>
>> Thanks,
>> Gao Xiang
>>
>>> ---
>>>  drivers/staging/erofs/inode.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/staging/erofs/inode.c b/drivers/staging/erofs/inode.c
>>> index 1433f25..2fe0f6d 100644
>>> --- a/drivers/staging/erofs/inode.c
>>> +++ b/drivers/staging/erofs/inode.c
>>> @@ -205,6 +205,7 @@ static int fill_inode(struct inode *inode, int isdir)
>>> S_ISFIFO(inode->i_mode) || S_ISSOCK(inode->i_mode)) {
>>> inode->i_op = _generic_iops;
>>> init_special_inode(inode, inode->i_mode, inode->i_rdev);
>>> +   goto out_unlock;
>>> } else {
>>> err = -EIO;
>>> goto out_unlock;
>>>   
> 
> .
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: erofs: Replace kzalloc(struct ..) with kzalloc(*ptr)

[Chao Yu]

On 2019/6/27 13:31, Shobhit Kukreti wrote:
> Resolve checkpatch warning:
> Prefer kzalloc(sizeof(*ptr)...) over kzalloc(sizeof(struct ..)
> 
> Signed-off-by: Shobhit Kukreti 

Reviewed-by: Chao Yu 

Thanks,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH RESEND] staging: erofs: return the error value if fill_inline_data() fails

[Chao Yu]

On 2019/6/26 11:30, Yue Hu wrote:
> From: Yue Hu 
> 
> We should consider the error returned by fill_inline_data() when filling
> last page in fill_inode(). If not getting inode will be successful even
> though last page is bad. That is illogical. Also change -EAGAIN to 0 in
> fill_inline_data() to stand for successful filling.
> 
> Signed-off-by: Yue Hu 

Reviewed-by: Chao Yu 

Thanks,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH RESEND] staging: erofs: remove unsupported ->datamode check in fill_inline_data()

[Chao Yu]

On 2019/6/26 11:28, Yue Hu wrote:
> From: Yue Hu 
> 
> Already check if ->datamode is supported in read_inode(), no need to check
> again in the next fill_inline_data() only called by fill_inode().
> 
> Signed-off-by: Yue Hu 

Reviewed-by: Chao Yu 

Thanks,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 5/8] staging: erofs: introduce generic decompression backend

[Chao Yu]

On 2019/6/21 18:42, Gao Xiang wrote:
> Hi Chao,
> 
> On 2019/6/21 17:46, Chao Yu wrote:
>> On 2019/6/21 0:07, Gao Xiang wrote:
>>> This patch adds a new generic decompression framework
>>> in order to replace the old LZ4-specific decompression code.
>>>
>>> Even though LZ4 is still the only supported algorithm, yet
>>> it is more cleaner and easy to integrate new algorithm than
>>> the old almost hard-coded decompression backend.
>>>
>>> Signed-off-by: Gao Xiang 
>>> ---
>>>  drivers/staging/erofs/Makefile   |   2 +-
>>>  drivers/staging/erofs/compress.h |  21 ++
>>>  drivers/staging/erofs/decompressor.c | 307 +++
>>>  3 files changed, 329 insertions(+), 1 deletion(-)
>>>  create mode 100644 drivers/staging/erofs/decompressor.c
>>>
>>> diff --git a/drivers/staging/erofs/Makefile b/drivers/staging/erofs/Makefile
>>> index 84b412c7a991..adeb5d6e2668 100644
>>> --- a/drivers/staging/erofs/Makefile
>>> +++ b/drivers/staging/erofs/Makefile
>>> @@ -9,5 +9,5 @@ obj-$(CONFIG_EROFS_FS) += erofs.o
>>>  ccflags-y += -I $(srctree)/$(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_vle_lz4.o zmap.o
>>> +erofs-$(CONFIG_EROFS_FS_ZIP) += unzip_vle.o unzip_vle_lz4.o zmap.o 
>>> decompressor.o
>>>  
>>> diff --git a/drivers/staging/erofs/compress.h 
>>> b/drivers/staging/erofs/compress.h
>>> index 1dcfc3b35118..ebeccb1f4eae 100644
>>> --- a/drivers/staging/erofs/compress.h
>>> +++ b/drivers/staging/erofs/compress.h
>>> @@ -9,6 +9,24 @@
>>>  #ifndef __EROFS_FS_COMPRESS_H
>>>  #define __EROFS_FS_COMPRESS_H
>>>  
>>> +#include "internal.h"
>>> +
>>> +enum {
>>> +   Z_EROFS_COMPRESSION_SHIFTED = Z_EROFS_COMPRESSION_MAX,
>>> +   Z_EROFS_COMPRESSION_RUNTIME_MAX
>>> +};
>>> +
>>> +struct z_erofs_decompress_req {
>>> +   struct page **in, **out;
>>> +
>>> +   unsigned short pageofs_out;
>>> +   unsigned int inputsize, outputsize;
>>> +
>>> +   /* indicate the algorithm will be used for decompression */
>>> +   unsigned int alg;
>>> +   bool inplace_io, partial_decoding;
>>> +};
>>> +
>>>  /*
>>>   * - 0x5A110C8D ('sallocated', Z_EROFS_MAPPING_STAGING) -
>>>   * used to mark temporary allocated pages from other
>>> @@ -36,5 +54,8 @@ static inline bool z_erofs_put_stagingpage(struct 
>>> list_head *pagepool,
>>> return true;
>>>  }
>>>  
>>> +int z_erofs_decompress(struct z_erofs_decompress_req *rq,
>>> +  struct list_head *pagepool);
>>> +
>>>  #endif
>>>  
>>> diff --git a/drivers/staging/erofs/decompressor.c 
>>> b/drivers/staging/erofs/decompressor.c
>>> new file mode 100644
>>> index ..c68d17b579e0
>>> --- /dev/null
>>> +++ b/drivers/staging/erofs/decompressor.c
>>> @@ -0,0 +1,307 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * linux/drivers/staging/erofs/decompressor.c
>>> + *
>>> + * Copyright (C) 2019 HUAWEI, Inc.
>>> + * http://www.huawei.com/
>>> + * Created by Gao Xiang 
>>> + */
>>> +#include "compress.h"
>>> +#include 
>>> +
>>> +#ifndef LZ4_DISTANCE_MAX   /* history window size */
>>> +#define LZ4_DISTANCE_MAX 65535 /* set to maximum value by default */
>>> +#endif
>>> +
>>> +#define LZ4_MAX_DISTANCE_PAGES DIV_ROUND_UP(LZ4_DISTANCE_MAX, 
>>> PAGE_SIZE)
>>> +
>>> +struct z_erofs_decompressor {
>>> +   /*
>>> +* if destpages have sparsed pages, fill them with bounce pages.
>>> +* it also check whether destpages indicate continuous physical memory.
>>> +*/
>>> +   int (*prepare_destpages)(struct z_erofs_decompress_req *rq,
>>> +struct list_head *pagepool);
>>> +   int (*decompress)(struct z_erofs_decompress_req *rq, u8 *out);
>>> +   char *name;
>>> +};
>>> +
>>> +static int lz4_prepare_destpages(struct z_erofs_decompress_req *rq,
>>> +struct list_head *pagepool)
>>> +{
>>> +   const unsigned int nr =
>>> +   PAGE_A

Re: [PATCH v3 0/8] staging: erofs: decompression inplace approach

[Chao Yu]

On 2019/6/24 15:22, Gao Xiang wrote:
> This is patch v3 of erofs decompression inplace approach, which is sent
> out by my personal email since I'm out of office to attend Open Source
> Summit China 2019 these days. No major change from PATCH v2 since no
> noticeable issue raised from landing to our products till now, mainly
> as a response to Chao's suggestions.
> 
> See the bottom lines which are taken from RFC PATCH v1 and describe
> the principle of these technologies.
> 
> The series is based on the latest staging-next since all dependencies
> have already been merged.
> 
> changelog from v2:
>  - wrap up some offsets as marcos;
>  - add some error handling for erofs_get_pcpubuf();
>  - move some decompression inplace stuffes from PATCH 5 -> 6.

Thanks for the update, those all changes look good to me. :)

Thanks,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 3/8] staging: erofs: move per-CPU buffers implementation to utils.c

[Chao Yu]

On 2019/6/24 15:22, Gao Xiang wrote:
> From: Gao Xiang 
> 
> This patch moves per-CPU buffers to utils.c in order for
> the upcoming generic decompression framework to use it.
> 
> Note that I tried to use generic per-CPU buffer or
> per-CPU page approaches to clean up further, but obvious
> performanace regression (about 2% for sequential read) was
> observed.
> 
> Therefore let's leave it as it is instead, just move
> to utils.c and I'll try to dig into the root cause later.
> 
> Signed-off-by: Gao Xiang 

Reviewed-by: Chao Yu 

Thanks,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 7/8] staging: erofs: switch to new decompression backend

[Chao Yu]

On 2019/6/21 0:07, Gao Xiang wrote:
> This patch integrates new decompression framework to
> erofs decompression path, and remove the old
> decompression implementation as well.
> 
> On kirin980 platform, sequential read is slightly
> improved to 778MiB/s after the new decompression
> backend is used.
> 
> Signed-off-by: Gao Xiang 

Reviewed-by: Chao Yu 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 8/8] staging: erofs: integrate decompression inplace

[Chao Yu]

On 2019/6/21 0:07, Gao Xiang wrote:
> Decompressor needs to know whether it's a partial
> or full decompression since only full decompression
> can be decompressed in-place.
> 
> On kirin980 platform, sequential read is finally
> increased to 812MiB/s after decompression inplace
> is enabled.
> 
> Signed-off-by: Gao Xiang 

Reviewed-by: Chao Yu 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 6/8] staging: erofs: introduce LZ4 decompression inplace

[Chao Yu]

On 2019/6/21 0:07, Gao Xiang wrote:
> compressed data will be usually loaded into last pages of
> the extent (the last page for 4k) for in-place decompression
> (more specifically, in-place IO), as ilustration below,
> 
>  start of compressed logical extent
>|  end of this logical extent
>|   |
>  __v___v
> ... |  page 6  |  page 7  |  page 8  |  page 9  | ...
> |__|__|__|__|
>. ^ .^
>. |compressed|
>. |   data   |
>.   ..
>|<  dstsize>||
>oend iend
>opip
> 
> Therefore, it's possible to do decompression inplace (thus no
> memcpy at all) if the margin is sufficient and safe enough [1],
> and it can be implemented only for fixed-size output compression
> compared with fixed-size input compression.
> 
> No memcpy for most of in-place IO (about 99% of enwik9) after
> decompression inplace is implemented and sequential read will
> be improved of course (see the following patches for test results).
> 
> [1] https://github.com/lz4/lz4/commit/b17f578a919b7e6b078cede2d52be29dd48c8e8c
> https://github.com/lz4/lz4/commit/5997e139f53169fa3a1c1b4418d2452a90b01602
> 
> Signed-off-by: Gao Xiang 

Reviewed-by: Chao Yu 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 5/8] staging: erofs: introduce generic decompression backend

[Chao Yu]

 } else if (!i) {
> + kaddr = page_address(page);
> + }
> + continue;
> + }
> + kaddr = NULL;
> +
> + k = find_first_bit(unused, LZ4_MAX_DISTANCE_PAGES);
> + if (k < LZ4_MAX_DISTANCE_PAGES) {
> + j = k;
> + get_page(availables[j]);
> + } else {
> + DBG_BUGON(availables[j]);
> +
> + if (!list_empty(pagepool)) {
> + availables[j] = lru_to_page(pagepool);
> + list_del([j]->lru);
> + DBG_BUGON(page_ref_count(availables[j]) != 1);
> + } else {
> + availables[j] = alloc_pages(GFP_KERNEL, 0);
> + if (!availables[j])
> + return -ENOMEM;
> + }
> + availables[j]->mapping = Z_EROFS_MAPPING_STAGING;

Could we use __stagingpage_alloc() instead opened codes, there is something
different in between them though.

Reviewed-by: Chao Yu 

Thanks,

> + }
> + rq->out[i] = availables[j];
> + __clear_bit(j, unused);
> + }
> + return kaddr ? 1 : 0;
> +}
> +
> +static void *generic_copy_inplace_data(struct z_erofs_decompress_req *rq,
> +u8 *src, unsigned int pageofs_in)
> +{
> + /*
> +  * if in-place decompression is ongoing, those decompressed
> +  * pages should be copied in order to avoid being overlapped.
> +  */
> + struct page **in = rq->in;
> + u8 *const tmp = erofs_get_pcpubuf(0);
> + u8 *tmpp = tmp;
> + unsigned int inlen = rq->inputsize - pageofs_in;
> + unsigned int count = min_t(uint, inlen, PAGE_SIZE - pageofs_in);
> +
> + while (tmpp < tmp + inlen) {
> + if (!src)
> + src = kmap_atomic(*in);
> + memcpy(tmpp, src + pageofs_in, count);
> + kunmap_atomic(src);
> + src = NULL;
> + tmpp += count;
> + pageofs_in = 0;
> + count = PAGE_SIZE;
> + ++in;
> + }
> + return tmp;
> +}
> +
> +static int lz4_decompress(struct z_erofs_decompress_req *rq, u8 *out)
> +{
> + unsigned int inputmargin, inlen;
> + u8 *src;
> + bool copied;
> + int ret;
> +
> + if (rq->inputsize > PAGE_SIZE)
> + return -ENOTSUPP;
> +
> + src = kmap_atomic(*rq->in);
> + inputmargin = 0;
> + while (!src[inputmargin & ~PAGE_MASK])
> + if (!(++inputmargin & ~PAGE_MASK))
> + break;
> +
> + if (inputmargin >= rq->inputsize) {
> + kunmap_atomic(src);
> + return -EIO;
> + }
> +
> + copied = false;
> + inlen = rq->inputsize - inputmargin;
> + if (rq->inplace_io) {
> + src = generic_copy_inplace_data(rq, src, inputmargin);
> + inputmargin = 0;
> + copied = true;
> + }
> +
> + ret = LZ4_decompress_safe_partial(src + inputmargin, out,
> +   inlen, rq->outputsize,
> +   rq->outputsize);
> + if (ret < 0) {
> + errln("%s, failed to decompress, in[%p, %u, %u] out[%p, %u]",
> +   __func__, src + inputmargin, inlen, inputmargin,
> +   out, rq->outputsize);
> + WARN_ON(1);
> + print_hex_dump(KERN_DEBUG, "[ in]: ", DUMP_PREFIX_OFFSET,
> +16, 1, src + inputmargin, inlen, true);
> + print_hex_dump(KERN_DEBUG, "[out]: ", DUMP_PREFIX_OFFSET,
> +16, 1, out, rq->outputsize, true);
> + ret = -EIO;
> + }
> +
> + if (copied)
> + erofs_put_pcpubuf(src);
> + else
> + kunmap_atomic(src);
> + return ret;
> +}
> +
> +static struct z_erofs_decompressor decompressors[] = {
> + [Z_EROFS_COMPRESSION_SHIFTED] = {
> + .name = "shifted"
> + },
> + [Z_EROFS_COMPRESSION_LZ4] = {
> + .prepare_destpages = lz4_prepare_destpages,
> + .decompress = lz4_decompress,
> + .name = "lz4"
> + },
> +};
> +
> +static void copy_from_pcpubuf(struct page **out, const char *dst,
> +   unsigned short pageofs_out,
> +   unsigned int output

Re: [PATCH v2 4/8] staging: erofs: move stagingpage operations to compress.h

[Chao Yu]

On 2019/6/21 0:07, Gao Xiang wrote:
> stagingpages are behaved as bounce pages for temporary use.
> Move to compress.h since the upcoming decompressor will
> allocate stagingpages as well.
> 
> Signed-off-by: Gao Xiang 

Reviewed-by: Chao Yu 

Thanks,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 3/8] staging: erofs: move per-CPU buffers implementation to utils.c

[Chao Yu]

On 2019/6/21 0:07, Gao Xiang wrote:
> +static inline void *erofs_get_pcpubuf(unsigned int pagenr)
> +{
> + return ERR_PTR(-ENOTSUPP);
> +}

[snip]

> + percpu_data = erofs_get_pcpubuf(0);

If erofs_get_pcpubuf may return error once EROFS_PCPUBUF_NR_PAGES equals to
zero, we'd better to check the error number here.

Thanks,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 2/8] staging: erofs: add compacted compression indexes support

[Chao Yu]

On 2019/6/21 0:07, Gao Xiang wrote:
> This patch aims at compacted compression indexes:
>  1) cleanup z_erofs_map_blocks_iter and move into zmap.c;
>  2) add compacted 4/2B decoding support.
> 
> On kirin980 platform, sequential read is increased about
> 6% (725MiB/s -> 770MiB/s) on enwik9 dataset if compacted 2B
> feature is enabled.
> 
> Signed-off-by: Gao Xiang 

Reviewed-by: Chao Yu 

> +static int vle_legacy_load_cluster_from_disk(struct z_erofs_maprecorder *m,
> +  unsigned long lcn)
> +{
> + struct inode *const inode = m->inode;
> + struct erofs_vnode *const vi = EROFS_V(inode);
> + const erofs_off_t ibase = iloc(EROFS_I_SB(inode), vi->nid);
> + const erofs_off_t pos = Z_EROFS_VLE_EXTENT_ALIGN(ibase +
> +  vi->inode_isize +
> +  vi->xattr_isize) +
> + 16 + lcn * sizeof(struct z_erofs_vle_decompressed_index);

use macro instead of raw number?

Thanks,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel