Re: kernel BUG at fs/erofs/inode.c:LINE!
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 more requirement, e.g. differ from runtime error vs corrupted 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: 000
Re: [PATCH 10/10] errno.h: Provide EFSCORRUPTED for everybody
On Sun, Nov 03, 2019 at 08:45:06PM -0500, 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 For EROFS part, Acked-by: Gao Xiang > --- > drivers/staging/exfat/exfat.h| 2 -- > fs/erofs/internal.h | 2 -- > fs/ext4/ext4.h | 1 - > fs/f2fs/f2fs.h | 1 - > fs/xfs/xfs_linux.h | 1 - > include/linux/jbd2.h | 1 - > include/uapi/asm-generic/errno.h | 1 + > 7 files changed, 1 insertion(+), 8 deletions(-) > > diff --git a/drivers/staging/exfat/exfat.h b/drivers/staging/exfat/exfat.h > index 72cf40e123de..58b091a077e8 100644 > --- a/drivers/staging/exfat/exfat.h > +++ b/drivers/staging/exfat/exfat.h > @@ -30,8 +30,6 @@ > #undef DEBUG > #endif > > -#define EFSCORRUPTED EUCLEAN /* Filesystem is corrupted */ > - > #define DENTRY_SIZE 32 /* dir entry size */ > #define DENTRY_SIZE_BITS 5 > > diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h > index 544a453f3076..3980026a8882 100644 > --- a/fs/erofs/internal.h > +++ b/fs/erofs/internal.h > @@ -425,7 +425,5 @@ static inline int z_erofs_init_zip_subsystem(void) { > return 0; } > static inline void z_erofs_exit_zip_subsystem(void) {} > #endif /* !CONFIG_EROFS_FS_ZIP */ > > -#define EFSCORRUPTEDEUCLEAN /* Filesystem is corrupted */ > - > #endif /* __EROFS_INTERNAL_H */ > > diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h > index 03db3e71676c..a86c2585457d 100644 > --- a/fs/ext4/ext4.h > +++ b/fs/ext4/ext4.h > @@ -3396,6 +3396,5 @@ static inline int ext4_buffer_uptodate(struct > buffer_head *bh) > #endif /* __KERNEL__ */ > > #define EFSBADCRCEBADMSG /* Bad CRC detected */ > -#define EFSCORRUPTED EUCLEAN /* Filesystem is corrupted */ > > #endif /* _EXT4_H */ > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > index 4024790028aa..04ebe77569a3 100644 > --- a/fs/f2fs/f2fs.h > +++ b/fs/f2fs/f2fs.h > @@ -3752,6 +3752,5 @@ static inline bool is_journalled_quota(struct > f2fs_sb_info *sbi) > } > > #define EFSBADCRCEBADMSG /* Bad CRC detected */ > -#define EFSCORRUPTED EUCLEAN /* Filesystem is corrupted */ > > #endif /* _LINUX_F2FS_H */ > diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h > index ca15105681ca..3409d02a7d21 100644 > --- a/fs/xfs/xfs_linux.h > +++ b/fs/xfs/xfs_linux.h > @@ -123,7 +123,6 @@ typedef __u32 xfs_nlink_t; > > #define ENOATTR ENODATA /* Attribute not found */ > #define EWRONGFS EINVAL /* Mount with wrong filesystem type */ > -#define EFSCORRUPTED EUCLEAN /* Filesystem is corrupted */ > #define EFSBADCRCEBADMSG /* Bad CRC detected */ > > #define SYNCHRONIZE()barrier() > diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h > index 603fbc4e2f70..69411d7e0431 100644 > --- a/include/linux/jbd2.h > +++ b/include/linux/jbd2.h > @@ -1657,6 +1657,5 @@ static inline tid_t > jbd2_get_latest_transaction(journal_t *journal) > #endif /* __KERNEL__ */ > > #define EFSBADCRCEBADMSG /* Bad CRC detected */ > -#define EFSCORRUPTED EUCLEAN /* Filesystem is corrupted */ > > #endif /* _LINUX_JBD2_H */ > diff --git a/include/uapi/asm-generic/errno.h > b/include/uapi/asm-generic/errno.h > index cf9c51ac49f9..1d5ffdf54cb0 100644 > --- a/include/uapi/asm-generic/errno.h > +++ b/include/uapi/asm-generic/errno.h > @@ -98,6 +98,7 @@ > #define EINPROGRESS 115 /* Operation now in progress */ > #define ESTALE 116 /* Stale file handle */ > #define EUCLEAN 117 /* Structure needs cleaning */ > +#define EFSCORRUPTEDEUCLEAN BTW, minor, how about adding some comments right after EFSCORRUPTED like the other error codes although it's now an alias... Just my personal thought. Thanks, Gao Xiang > #define ENOTNAM 118 /* Not a XENIX named type file */ > #define ENAVAIL 119 /* No XENIX semaphores available */ > #define EISNAM 120 /* Is a named type file */ > -- > 2.24.0.rc1 > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [GIT PULL] Staging/IIO driver patches for 5.4-rc1
On Thu, Sep 19, 2019 at 05:11:28AM +0800, Gao Xiang wrote: > Hi Christoph, > > On Wed, Sep 18, 2019 at 11:24:12AM -0700, Christoph Hellwig wrote: > > Just as a note of record: I don't think either file system move > > is a good idea. erofs still needs a lot of work, especially in > > interacting with the mm code like abusing page->mapping. > > I know what you mean, that is a known stuff in the TODO list, > Z_EROFS_MAPPING_STAGING page->mapping just be used as a temporary > page mark since page->private is already pointed to another > structure, It's now be marked as an non-movable and anon pseudo > mapping and most mm code just skip this case. Add a word, these pages are all non-LRU and short lifetime temporary pages (and need to differentiate with other NULL mapping pages [a lot of different type pages could have this intermediate state]). Alternate way is to use some page flag but that is what I really want to avoid this limited resource. For EROFS, it's widely deployment on each new HUAWEI mobile phones on the market this year and all old HUAWEI modile phones are still supported by upgrading to EROFS version and there are many pending new features for EROFS and a mature fixed-sized output compression subsystem in the future if more fs users have interested in that and I think it's good for whole linux ecosystem not just on a single filesystem upstreaming basis and we will continue working on this area. Thanks, Gao Xiang > > I think a better way is to use a real address_space structure for > page->mapping to point. It's easy to update but I need some time > to verify. If I am wrong, please point it out... > > Thanks, > Gao Xiang > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [GIT PULL] Staging/IIO driver patches for 5.4-rc1
Hi Christoph, On Wed, Sep 18, 2019 at 11:24:12AM -0700, Christoph Hellwig wrote: > Just as a note of record: I don't think either file system move > is a good idea. erofs still needs a lot of work, especially in > interacting with the mm code like abusing page->mapping. I know what you mean, that is a known stuff in the TODO list, Z_EROFS_MAPPING_STAGING page->mapping just be used as a temporary page mark since page->private is already pointed to another structure, It's now be marked as an non-movable and anon pseudo mapping and most mm code just skip this case. I think a better way is to use a real address_space structure for page->mapping to point. It's easy to update but I need some time to verify. If I am wrong, please point it out... Thanks, Gao Xiang ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: exfat: add exfat filesystem code to
Hi, On Tue, Sep 17, 2019 at 12:02:01PM +0900, Namjae Jeon wrote: > We are excited to see this happening and would like to state that we > appreciate > time and > effort which people put into upstreaming exfat. Thank you! > > However, if possible, can we step back a little bit and re-consider it? We > would prefer to > see upstream the code which we are currently using in our products - sdfat - > as > this can > be mutually benefitial from various points of view. (Only represent my personal views) I'd like to know the detailed commit history as an individual Android hobbyist. I noticed sdfat years ago and there is a difference from the previous exfat driver. I have no idea it's a good way to blindly keep the code from some opensource tar on some website. and so many forks on github (hard to know which one is more stable, cleaner or latest)... someone could take more time and play a role in that actively in the community and maybe draw a roadmap of this so I could study more and maybe contribute a little in my spare time. And I think if it permits, development on multiple branches could be avoided... If I am wrong, please ignore me... Thanks, Gao Xiang > > Thanks! > > > -- Forwarded message - > > : Ju Hyung Park > > Date: 2019?? 9?? 16?? (??) 3:49 > > Subject: Re: [PATCH] staging: exfat: add exfat filesystem code to > > To: Greg KH > > Cc: , , > fsde...@vger.kernel.org>, , Valdis Kletnieks > > > > > > > > Hi Greg, > > > > On Sun, Sep 15, 2019 at 10:54 PM Greg KH wrote: > > > Note, this just showed up publically on August 12, where were you with > > > all of this new code before then? :) > > > > My sdFAT port, exfat-nofuse and the one on the staging tree, were all > > made by Samsung. > > And unless you guys had a chance to talk to Samsung developers > > directly, those all share the same faith of lacking proper development > > history. > > > > The source I used was from http://opensource.samsung.com, which > > provides kernel sources as tar.gz files. > > There is no code history available. > > > > > For the in-kernel code, we would have to rip out all of the work you did > > > for all older kernels, so that's a non-starter right there. > > > > I'm aware. > > I'm just letting mainline know that there is potentially another (much > > better) base that could be upstreamed. > > > > If you want me to rip out older kernel support for upstreaming, I'm > > more than happy to do so. > > > > > As for what codebase to work off of, I don't want to say it is too late, > > > but really, this shows up from nowhere and we had to pick something so > > > we found the best we could at that point in time. > > > > To be honest, whole public exFAT sources are all from nowhere unless > > you had internal access to Samsung's development archive. > > The one in the current staging tree isn't any better. > > > > I'm not even sure where the staging driver is from, actually. > > > > Samsung used the 1.2.x versioning until they switched to a new code > > base - sdFAT. > > The one in the staging tree is marked version 1.3.0(exfat_super.c). > > I failed to find anything 1.3.x from Samsung's public kernel sources. > > > > The last time exFAT 1.2.x was used was in Galaxy S7(released in 2016). > > Mine was originally based on sdFAT 2.1.10, used in Galaxy S10(released > > in March 2019) and it just got updated to 2.2.0, used in Galaxy > > Note10(released in August 2019). > > > > > Is there anything specific in the codebase you have now, that is lacking > > > in the in-kernel code? Old-kernel-support doesn't count here, as we > > > don't care about that as it is not applicable. But functionality does > > > matter, what has been added here that we can make use of? > > > > This is more of a suggestion of > > "Let's base on a *much more recent* snapshot for the community to work on", > > since the current one on the staging tree also lacks development history. > > > > The diff is way too big to even start understanding the difference. > > > > > > With that said though, I do have some vague but real reason as to why > > sdFAT base is better. > > > > With some major Android vendors showing interests in supporting exFAT, > > Motorola notably published their work on public Git repository with > > full development history(the only vendor to do this that I'm aware > > of). > > Commits like this: > > https://github.com/MotorolaMobilityLLC
Re: [PATCH v2 00/25] erofs: patchset addressing Christoph's comments
Hi Christoph, On Wed, Sep 04, 2019 at 11:27:11AM +0800, Chao Yu wrote: > 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, Could we submit these patches if these patches look good... Since I'd like to work based on these patches, it makes a difference to the current code though... Thanks a lot! Thanks, Gao Xiang > > > > > 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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] erofs: using switch-case while checking the inode type.
On Wed, Sep 04, 2019 at 08:31:34AM +0200, Greg KH wrote: > On Wed, Sep 04, 2019 at 10:12:47AM +0800, Gao Xiang wrote: > > Hi Greg, > > > > On Fri, Aug 30, 2019 at 10:22:33PM +0800, Gao Xiang wrote: > > > On Fri, Aug 30, 2019 at 07:59:48PM +0800, Gao Xiang wrote: > > > > Hi Pratik, > > > > > > > > The subject line could be better as '[PATCH v2] xx'... > > > > > > > > On Fri, Aug 30, 2019 at 03:26:15PM +0530, Pratik Shinde wrote: > > > > > while filling the linux inode, using switch-case statement to check > > > > > the type of inode. > > > > > switch-case statement looks more clean here. > > > > > > > > > > Signed-off-by: Pratik Shinde > > > > > > > > I have no problem of this patch, and I will do a test and reply > > > > you "Reviewed-by:" in hours (I'm doing another patchset to resolve > > > > what Christoph suggested again)... > > > > > > Reviewed-by: Gao Xiang > > > > ping.. could you kindly merge this patch, the following patchset > > has dependency on it... > > Will go do that now, sorry for the delay. Thanks Greg... Thanks, Gao Xiang > > greg k-h ___ 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
Hi Christoph, On Wed, Sep 04, 2019 at 07:16:55AM +0200, Christoph Hellwig wrote: > On Wed, Sep 04, 2019 at 10:08:47AM +0800, 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. > > > > Comments and suggestions are welcome... > > Do you have a git branch available somewhere containing the state > with all these patches applied? here you are... git://git.kernel.org/pub/scm/linux/kernel/git/xiang/linux.git -b erofs-experimental Thanks, Gao Xiang ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] erofs: using switch-case while checking the inode type.
Hi Greg, On Fri, Aug 30, 2019 at 10:22:33PM +0800, Gao Xiang wrote: > On Fri, Aug 30, 2019 at 07:59:48PM +0800, Gao Xiang wrote: > > Hi Pratik, > > > > The subject line could be better as '[PATCH v2] xx'... > > > > On Fri, Aug 30, 2019 at 03:26:15PM +0530, Pratik Shinde wrote: > > > while filling the linux inode, using switch-case statement to check > > > the type of inode. > > > switch-case statement looks more clean here. > > > > > > Signed-off-by: Pratik Shinde > > > > I have no problem of this patch, and I will do a test and reply > > you "Reviewed-by:" in hours (I'm doing another patchset to resolve > > what Christoph suggested again)... > > Reviewed-by: Gao Xiang ping.. could you kindly merge this patch, the following patchset has dependency on it... Thanks, Gao Xiang > > Thanks, > Gao Xiang > > > > > Thanks, > > Gao Xiang > > > > > --- > > > fs/erofs/inode.c | 18 -- > > > 1 file changed, 12 insertions(+), 6 deletions(-) > > > > > > diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c > > > index 80f4fe9..6336cc1 100644 > > > --- a/fs/erofs/inode.c > > > +++ b/fs/erofs/inode.c > > > @@ -190,22 +190,28 @@ static int fill_inode(struct inode *inode, int > > > isdir) > > > err = read_inode(inode, data + ofs); > > > if (!err) { > > > /* setup the new inode */ > > > - if (S_ISREG(inode->i_mode)) { > > > + switch (inode->i_mode & S_IFMT) { > > > + case S_IFREG: > > > inode->i_op = _generic_iops; > > > inode->i_fop = _ro_fops; > > > - } else if (S_ISDIR(inode->i_mode)) { > > > + break; > > > + case S_IFDIR: > > > inode->i_op = _dir_iops; > > > inode->i_fop = _dir_fops; > > > - } else if (S_ISLNK(inode->i_mode)) { > > > + break; > > > + case S_IFLNK: > > > /* by default, page_get_link is used for symlink */ > > > inode->i_op = _symlink_iops; > > > inode_nohighmem(inode); > > > - } else if (S_ISCHR(inode->i_mode) || S_ISBLK(inode->i_mode) || > > > - S_ISFIFO(inode->i_mode) || S_ISSOCK(inode->i_mode)) { > > > + break; > > > + case S_IFCHR: > > > + case S_IFBLK: > > > + case S_IFIFO: > > > + case S_IFSOCK: > > > inode->i_op = _generic_iops; > > > init_special_inode(inode, inode->i_mode, inode->i_rdev); > > > goto out_unlock; > > > - } else { > > > + default: > > > err = -EFSCORRUPTED; > > > goto out_unlock; > > > } > > > -- > > > 2.9.3 > > > > > > ___ > > > devel mailing list > > > de...@linuxdriverproject.org > > > http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 18/25] erofs: add "erofs_" prefix for common and short functions
Add erofs_ prefix to free_inode, alloc_inode, ... Reported-by: Christoph Hellwig Signed-off-by: Gao Xiang --- fs/erofs/data.c | 4 ++-- fs/erofs/decompressor.c | 22 +++--- fs/erofs/inode.c| 8 fs/erofs/internal.h | 2 +- fs/erofs/namei.c| 12 ++-- fs/erofs/super.c| 40 fs/erofs/zdata.c| 19 ++- 7 files changed, 54 insertions(+), 53 deletions(-) diff --git a/fs/erofs/data.c b/fs/erofs/data.c index 70b1e353756e..3ce87a88452a 100644 --- a/fs/erofs/data.c +++ b/fs/erofs/data.c @@ -9,7 +9,7 @@ #include -static inline void read_endio(struct bio *bio) +static void erofs_readendio(struct bio *bio) { struct super_block *const sb = bio->bi_private; struct bio_vec *bvec; @@ -45,7 +45,7 @@ static struct bio *erofs_grab_raw_bio(struct super_block *sb, { struct bio *bio = bio_alloc(GFP_NOIO, nr_pages); - bio->bi_end_io = read_endio; + bio->bi_end_io = erofs_readendio; bio_set_dev(bio, sb->s_bdev); bio->bi_iter.bi_sector = (sector_t)blkaddr << LOG_SECTORS_PER_BLOCK; bio->bi_private = sb; diff --git a/fs/erofs/decompressor.c b/fs/erofs/decompressor.c index 555c04730f87..8ed38504a9f1 100644 --- a/fs/erofs/decompressor.c +++ b/fs/erofs/decompressor.c @@ -32,8 +32,8 @@ static bool use_vmap; module_param(use_vmap, bool, 0444); MODULE_PARM_DESC(use_vmap, "Use vmap() instead of vm_map_ram() (default 0)"); -static int lz4_prepare_destpages(struct z_erofs_decompress_req *rq, -struct list_head *pagepool) +static int z_erofs_lz4_prepare_destpages(struct z_erofs_decompress_req *rq, +struct list_head *pagepool) { const unsigned int nr = PAGE_ALIGN(rq->pageofs_out + rq->outputsize) >> PAGE_SHIFT; @@ -114,7 +114,7 @@ static void *generic_copy_inplace_data(struct z_erofs_decompress_req *rq, return tmp; } -static int lz4_decompress(struct z_erofs_decompress_req *rq, u8 *out) +static int z_erofs_lz4_decompress(struct z_erofs_decompress_req *rq, u8 *out) { unsigned int inputmargin, inlen; u8 *src; @@ -188,8 +188,8 @@ static struct z_erofs_decompressor decompressors[] = { .name = "shifted" }, [Z_EROFS_COMPRESSION_LZ4] = { - .prepare_destpages = lz4_prepare_destpages, - .decompress = lz4_decompress, + .prepare_destpages = z_erofs_lz4_prepare_destpages, + .decompress = z_erofs_lz4_decompress, .name = "lz4" }, }; @@ -247,8 +247,8 @@ static void erofs_vunmap(const void *mem, unsigned int count) vunmap(mem); } -static int decompress_generic(struct z_erofs_decompress_req *rq, - struct list_head *pagepool) +static int z_erofs_decompress_generic(struct z_erofs_decompress_req *rq, + struct list_head *pagepool) { const unsigned int nrpages_out = PAGE_ALIGN(rq->pageofs_out + rq->outputsize) >> PAGE_SHIFT; @@ -308,8 +308,8 @@ static int decompress_generic(struct z_erofs_decompress_req *rq, return ret; } -static int shifted_decompress(const struct z_erofs_decompress_req *rq, - struct list_head *pagepool) +static int z_erofs_shifted_transform(const struct z_erofs_decompress_req *rq, +struct list_head *pagepool) { const unsigned int nrpages_out = PAGE_ALIGN(rq->pageofs_out + rq->outputsize) >> PAGE_SHIFT; @@ -353,7 +353,7 @@ int z_erofs_decompress(struct z_erofs_decompress_req *rq, struct list_head *pagepool) { if (rq->alg == Z_EROFS_COMPRESSION_SHIFTED) - return shifted_decompress(rq, pagepool); - return decompress_generic(rq, pagepool); + return z_erofs_shifted_transform(rq, pagepool); + return z_erofs_decompress_generic(rq, pagepool); } diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c index 8d496adbeaea..384905e0677c 100644 --- a/fs/erofs/inode.c +++ b/fs/erofs/inode.c @@ -9,7 +9,7 @@ #include /* no locking */ -static int read_inode(struct inode *inode, void *data) +static int erofs_read_inode(struct inode *inode, void *data) { struct erofs_inode *vi = EROFS_I(inode); struct erofs_inode_compact *dic = data; @@ -163,7 +163,7 @@ static int erofs_fill_symlink(struct inode *inode, void *data, return 0; } -static int fill_inode(struct inode *inode, int isdir) +static int erofs_fill_inode(struct inode *inode, int isdir) { struct erofs_sb_info *sbi = EROFS_SB(inode->i_sb); struct erofs_inode *vi = EROFS_I(inode); @@ -193,7 +193,7 @@ static int fill_inode(struct inode *inode, int
[PATCH v2 20/25] erofs: kill use_vmap module parameter
As Christoph said [1], "vm_map_ram is supposed to generally behave better. So if it doesn't please report that that to the arch maintainer and linux-mm so that they can look into the issue. Having user make choices of deep down kernel internals is just a horrible interface. Please talk to maintainers of other bits of the kernel if you see issues and / or need enhancements. " Let's redo the previous conclusion and kill the vmap approach. [1] https://lore.kernel.org/r/20190830165533.ga10...@infradead.org/ Reported-by: Christoph Hellwig Signed-off-by: Gao Xiang --- Documentation/filesystems/erofs.txt | 4 --- fs/erofs/decompressor.c | 46 - 2 files changed, 13 insertions(+), 37 deletions(-) diff --git a/Documentation/filesystems/erofs.txt b/Documentation/filesystems/erofs.txt index c3b5f603b2b6..b0c085326e2e 100644 --- a/Documentation/filesystems/erofs.txt +++ b/Documentation/filesystems/erofs.txt @@ -67,10 +67,6 @@ cache_strategy=%s Select a strategy for cached decompression from now on: It still does in-place I/O decompression for the rest compressed physical clusters. -Module parameters -= -use_vmap=[0|1] Use vmap() instead of vm_map_ram() (default 0). - On-disk details === diff --git a/fs/erofs/decompressor.c b/fs/erofs/decompressor.c index 8ed38504a9f1..37177d49d125 100644 --- a/fs/erofs/decompressor.c +++ b/fs/erofs/decompressor.c @@ -28,10 +28,6 @@ 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)"); - static int z_erofs_lz4_prepare_destpages(struct z_erofs_decompress_req *rq, struct list_head *pagepool) { @@ -221,32 +217,6 @@ static void copy_from_pcpubuf(struct page **out, const char *dst, } } -static void *erofs_vmap(struct page **pages, unsigned int count) -{ - int i = 0; - - if (use_vmap) - return vmap(pages, count, VM_MAP, PAGE_KERNEL); - - 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; -} - -static void erofs_vunmap(const void *mem, unsigned int count) -{ - if (!use_vmap) - vm_unmap_ram(mem, count); - else - vunmap(mem); -} - static int z_erofs_decompress_generic(struct z_erofs_decompress_req *rq, struct list_head *pagepool) { @@ -255,7 +225,7 @@ static int z_erofs_decompress_generic(struct z_erofs_decompress_req *rq, const struct z_erofs_decompressor *alg = decompressors + rq->alg; unsigned int dst_maptype; void *dst; - int ret; + int ret, i; if (nrpages_out == 1 && !rq->inplace_io) { DBG_BUGON(!*rq->out); @@ -293,9 +263,19 @@ static int z_erofs_decompress_generic(struct z_erofs_decompress_req *rq, goto dstmap_out; } - dst = erofs_vmap(rq->out, nrpages_out); + i = 0; + while (1) { + dst = vm_map_ram(rq->out, nrpages_out, -1, PAGE_KERNEL); + + /* retry two more times (totally 3 times) */ + if (dst || ++i >= 3) + break; + vm_unmap_aliases(); + } + if (!dst) return -ENOMEM; + dst_maptype = 2; dstmap_out: @@ -304,7 +284,7 @@ static int z_erofs_decompress_generic(struct z_erofs_decompress_req *rq, if (!dst_maptype) kunmap_atomic(dst); else if (dst_maptype == 2) - erofs_vunmap(dst, nrpages_out); + vm_unmap_ram(dst, nrpages_out); return ret; } -- 2.17.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 22/25] erofs: rename errln/infoln/debugln to erofs_{err, info, dbg}
Add prefix "erofs_" to these functions and print sb->s_id as a prefix to erofs_{err, info} so that the user knows which file system is affected. Reported-by: Christoph Hellwig Signed-off-by: Gao Xiang --- fs/erofs/data.c | 10 +++-- fs/erofs/decompressor.c | 5 +-- fs/erofs/dir.c | 17 + fs/erofs/inode.c| 31 --- fs/erofs/internal.h | 14 +-- fs/erofs/namei.c| 9 +++-- fs/erofs/super.c| 83 +++-- fs/erofs/xattr.c| 8 ++-- fs/erofs/zdata.c| 13 --- fs/erofs/zdata.h| 2 +- fs/erofs/zmap.c | 37 ++ 11 files changed, 139 insertions(+), 90 deletions(-) diff --git a/fs/erofs/data.c b/fs/erofs/data.c index b5f5b8592d14..eb7bbae89ed0 100644 --- a/fs/erofs/data.c +++ b/fs/erofs/data.c @@ -137,8 +137,9 @@ static int erofs_map_blocks_flatmode(struct inode *inode, /* inline data should be located in one meta block */ if (erofs_blkoff(map->m_pa) + map->m_plen > PAGE_SIZE) { - errln("inline data cross block boundary @ nid %llu", - vi->nid); + erofs_err(inode->i_sb, + "inline data cross block boundary @ nid %llu", + vi->nid); DBG_BUGON(1); err = -EFSCORRUPTED; goto err_out; @@ -146,8 +147,9 @@ static int erofs_map_blocks_flatmode(struct inode *inode, map->m_flags |= EROFS_MAP_META; } else { - errln("internal error @ nid: %llu (size %llu), m_la 0x%llx", - vi->nid, inode->i_size, map->m_la); + erofs_err(inode->i_sb, + "internal error @ nid: %llu (size %llu), m_la 0x%llx", + vi->nid, inode->i_size, map->m_la); DBG_BUGON(1); err = -EIO; goto err_out; diff --git a/fs/erofs/decompressor.c b/fs/erofs/decompressor.c index 37177d49d125..19f89f9fb10c 100644 --- a/fs/erofs/decompressor.c +++ b/fs/erofs/decompressor.c @@ -161,9 +161,8 @@ static int z_erofs_lz4_decompress(struct z_erofs_decompress_req *rq, u8 *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); + erofs_err(rq->sb, "failed to decompress, in[%u, %u] out[%u]", + inlen, inputmargin, rq->outputsize); WARN_ON(1); print_hex_dump(KERN_DEBUG, "[ in]: ", DUMP_PREFIX_OFFSET, 16, 1, src + inputmargin, inlen, true); diff --git a/fs/erofs/dir.c b/fs/erofs/dir.c index a032c8217071..d28c623dfef9 100644 --- a/fs/erofs/dir.c +++ b/fs/erofs/dir.c @@ -16,8 +16,8 @@ static void debug_one_dentry(unsigned char d_type, const char *de_name, memcpy(dbg_namebuf, de_name, de_namelen); dbg_namebuf[de_namelen] = '\0'; - debugln("found dirent %s de_len %u d_type %d", dbg_namebuf, - de_namelen, d_type); + erofs_dbg("found dirent %s de_len %u d_type %d", dbg_namebuf, + de_namelen, d_type); #endif } @@ -47,7 +47,8 @@ static int erofs_fill_dentries(struct inode *dir, struct dir_context *ctx, /* a corrupted entry is found */ if (nameoff + de_namelen > maxsize || de_namelen > EROFS_NAME_LEN) { - errln("bogus dirent @ nid %llu", EROFS_I(dir)->nid); + erofs_err(dir->i_sb, "bogus dirent @ nid %llu", + EROFS_I(dir)->nid); DBG_BUGON(1); return -EFSCORRUPTED; } @@ -84,8 +85,9 @@ static int erofs_readdir(struct file *f, struct dir_context *ctx) err = -ENOMEM; break; } else if (IS_ERR(dentry_page)) { - errln("fail to readdir of logical block %u of nid %llu", - i, EROFS_I(dir)->nid); + erofs_err(dir->i_sb, + "fail to readdir of logical block %u of nid %llu", + i, EROFS_I(dir)->nid); err = -EFSCORRUPTED; break; } @@ -96,8 +98,9 @@ static int erofs_readdir(struct file *f, struct dir_context *ctx) i
[PATCH v2 25/25] erofs: use read_cache_page_gfp for erofs_get_meta_page
As Christoph said [1], "I'd much prefer to just use read_cache_page_gfp, and live with the fact that this allocates bufferheads behind you for now. I'll try to speed up my attempts to get rid of the buffer heads on the block device mapping instead. " This simplifies the code a lot and a minor thing is "no REQ_META (e.g. for blktrace) on metadata at all..." [1] https://lore.kernel.org/r/20190903153704.ga2...@infradead.org/ Reported-by: Christoph Hellwig Signed-off-by: Gao Xiang --- fs/erofs/data.c | 70 +++-- 1 file changed, 9 insertions(+), 61 deletions(-) diff --git a/fs/erofs/data.c b/fs/erofs/data.c index eb7bbae89ed0..8a9fcbd0e8ac 100644 --- a/fs/erofs/data.c +++ b/fs/erofs/data.c @@ -32,71 +32,13 @@ static void erofs_readendio(struct bio *bio) bio_put(bio); } -static struct bio *erofs_grab_raw_bio(struct super_block *sb, - erofs_blk_t blkaddr, - unsigned int nr_pages, - bool ismeta) -{ - struct bio *bio = bio_alloc(GFP_NOIO, nr_pages); - - bio->bi_end_io = erofs_readendio; - bio_set_dev(bio, sb->s_bdev); - bio->bi_iter.bi_sector = (sector_t)blkaddr << LOG_SECTORS_PER_BLOCK; - if (ismeta) - bio->bi_opf = REQ_OP_READ | REQ_META; - else - bio->bi_opf = REQ_OP_READ; - - return bio; -} - struct page *erofs_get_meta_page(struct super_block *sb, erofs_blk_t blkaddr) { struct inode *const bd_inode = sb->s_bdev->bd_inode; struct address_space *const mapping = bd_inode->i_mapping; - const gfp_t gfp = mapping_gfp_constraint(mapping, ~__GFP_FS); - struct page *page; - int err; - -repeat: - page = find_or_create_page(mapping, blkaddr, gfp); - if (!page) - return ERR_PTR(-ENOMEM); - - DBG_BUGON(!PageLocked(page)); - - if (!PageUptodate(page)) { - struct bio *bio; - - bio = erofs_grab_raw_bio(sb, blkaddr, 1, true); - - if (bio_add_page(bio, page, PAGE_SIZE, 0) != PAGE_SIZE) { - err = -EFAULT; - goto err_out; - } - - submit_bio(bio); - lock_page(page); - /* this page has been truncated by others */ - if (page->mapping != mapping) { - unlock_page(page); - put_page(page); - goto repeat; - } - - /* more likely a read error */ - if (!PageUptodate(page)) { - err = -EIO; - goto err_out; - } - } - return page; - -err_out: - unlock_page(page); - put_page(page); - return ERR_PTR(err); + return read_cache_page_gfp(mapping, blkaddr, + mapping_gfp_constraint(mapping, ~__GFP_FS)); } static int erofs_map_blocks_flatmode(struct inode *inode, @@ -272,7 +214,13 @@ static inline struct bio *erofs_read_raw_page(struct bio *bio, if (nblocks > BIO_MAX_PAGES) nblocks = BIO_MAX_PAGES; - bio = erofs_grab_raw_bio(sb, blknr, nblocks, false); + bio = bio_alloc(GFP_NOIO, nblocks); + + bio->bi_end_io = erofs_readendio; + bio_set_dev(bio, sb->s_bdev); + bio->bi_iter.bi_sector = (sector_t)blknr << + LOG_SECTORS_PER_BLOCK; + bio->bi_opf = REQ_OP_READ; } err = bio_add_page(bio, page, PAGE_SIZE, 0); -- 2.17.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 16/25] erofs: kill prio and nofail of erofs_get_meta_page()
As Christoph pointed out [1], "Why is there __erofs_get_meta_page with the two weird booleans instead of a single erofs_get_meta_page that gets and gfp_t for additional flags and an unsigned int for additional bio op flags." And since all callers can handle errors, let's kill prio and nofail and erofs_get_inline_page() now. [1] https://lore.kernel.org/r/20190830162812.ga10...@infradead.org/ Reported-by: Christoph Hellwig Signed-off-by: Gao Xiang --- fs/erofs/data.c | 25 ++--- fs/erofs/inode.c| 2 +- fs/erofs/internal.h | 18 +- fs/erofs/xattr.c| 13 ++--- fs/erofs/zmap.c | 4 ++-- 5 files changed, 16 insertions(+), 46 deletions(-) diff --git a/fs/erofs/data.c b/fs/erofs/data.c index 0136ea117934..28eda71bb1a9 100644 --- a/fs/erofs/data.c +++ b/fs/erofs/data.c @@ -51,25 +51,19 @@ static struct bio *erofs_grab_raw_bio(struct super_block *sb, return bio; } -/* prio -- true is used for dir */ -struct page *__erofs_get_meta_page(struct super_block *sb, - erofs_blk_t blkaddr, bool prio, bool nofail) +struct page *erofs_get_meta_page(struct super_block *sb, erofs_blk_t blkaddr) { struct inode *const bd_inode = sb->s_bdev->bd_inode; struct address_space *const mapping = bd_inode->i_mapping; - /* prefer retrying in the allocator to blindly looping below */ - const gfp_t gfp = mapping_gfp_constraint(mapping, ~__GFP_FS) | - (nofail ? __GFP_NOFAIL : 0); - unsigned int io_retries = nofail ? EROFS_IO_MAX_RETRIES_NOFAIL : 0; + const gfp_t gfp = mapping_gfp_constraint(mapping, ~__GFP_FS); struct page *page; int err; repeat: page = find_or_create_page(mapping, blkaddr, gfp); - if (!page) { - DBG_BUGON(nofail); + if (!page) return ERR_PTR(-ENOMEM); - } + DBG_BUGON(!PageLocked(page)); if (!PageUptodate(page)) { @@ -82,14 +76,11 @@ struct page *__erofs_get_meta_page(struct super_block *sb, goto err_out; } - __submit_bio(bio, REQ_OP_READ, -REQ_META | (prio ? REQ_PRIO : 0)); - + __submit_bio(bio, REQ_OP_READ, REQ_META); lock_page(page); /* this page has been truncated by others */ if (page->mapping != mapping) { -unlock_repeat: unlock_page(page); put_page(page); goto repeat; @@ -97,10 +88,6 @@ struct page *__erofs_get_meta_page(struct super_block *sb, /* more likely a read error */ if (!PageUptodate(page)) { - if (io_retries) { - --io_retries; - goto unlock_repeat; - } err = -EIO; goto err_out; } @@ -251,7 +238,7 @@ static inline struct bio *erofs_read_raw_page(struct bio *bio, DBG_BUGON(map.m_plen > PAGE_SIZE); - ipage = erofs_get_meta_page(inode->i_sb, blknr, 0); + ipage = erofs_get_meta_page(inode->i_sb, blknr); if (IS_ERR(ipage)) { err = PTR_ERR(ipage); diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c index 770f3259c862..8d496adbeaea 100644 --- a/fs/erofs/inode.c +++ b/fs/erofs/inode.c @@ -182,7 +182,7 @@ static int fill_inode(struct inode *inode, int isdir) debugln("%s, reading inode nid %llu at %u of blkaddr %u", __func__, vi->nid, ofs, blkaddr); - page = erofs_get_meta_page(inode->i_sb, blkaddr, isdir); + page = erofs_get_meta_page(inode->i_sb, blkaddr); if (IS_ERR(page)) { errln("failed to get inode (nid: %llu) page, err %ld", diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h index 000ea92b36a3..90c62fb5f80d 100644 --- a/fs/erofs/internal.h +++ b/fs/erofs/internal.h @@ -258,8 +258,6 @@ static inline int erofs_wait_on_workgroup_freezed(struct erofs_workgroup *grp) #error erofs cannot be used in this platform #endif -#define EROFS_IO_MAX_RETRIES_NOFAIL 5 - #define ROOT_NID(sb) ((sb)->root_nid) #define erofs_blknr(addr) ((addr) / EROFS_BLKSIZ) @@ -418,24 +416,10 @@ static inline void __submit_bio(struct bio *bio, unsigned int op, submit_bio(bio); } -struct page *__erofs_get_meta_page(struct super_block *sb, erofs_blk_t blkaddr, - bool prio, bool nofail); - -static inline struct page *erofs_get_meta_page(struct super_block *sb, - erofs_blk_t blkaddr, bool prio) -{ - return __erofs_get_meta_page(sb, blkaddr, prio, false); -} +struct page *erofs_get_meta_page(struct super_block *sb, erofs_blk_t blkaddr); int
[PATCH v2 09/25] erofs: use erofs_inode naming
As Christoph suggested [1], "Why is this called vnode instead of inode? That seems like a rather odd naming for a Linux file system." [1] https://lore.kernel.org/r/20190829101545.gc20...@infradead.org/ Reported-by: Christoph Hellwig Signed-off-by: Gao Xiang --- fs/erofs/data.c | 8 fs/erofs/dir.c | 6 +++--- fs/erofs/inode.c | 12 ++-- fs/erofs/internal.h | 14 +++--- fs/erofs/namei.c | 2 +- fs/erofs/super.c | 10 +- fs/erofs/xattr.c | 18 +- fs/erofs/xattr.h | 4 ++-- fs/erofs/zdata.c | 9 +++-- fs/erofs/zmap.c | 28 ++-- include/trace/events/erofs.h | 14 +++--- 11 files changed, 61 insertions(+), 64 deletions(-) diff --git a/fs/erofs/data.c b/fs/erofs/data.c index 4d9b07991d07..be11b5ea9d2e 100644 --- a/fs/erofs/data.c +++ b/fs/erofs/data.c @@ -112,7 +112,7 @@ static int erofs_map_blocks_flatmode(struct inode *inode, int err = 0; erofs_blk_t nblocks, lastblk; u64 offset = map->m_la; - struct erofs_vnode *vi = EROFS_V(inode); + struct erofs_inode *vi = EROFS_I(inode); bool tailendpacking = (vi->datalayout == EROFS_INODE_FLAT_INLINE); trace_erofs_map_blocks_flatmode_enter(inode, map, flags); @@ -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 (erofs_inode_is_data_compressed(EROFS_V(inode)->datalayout)) { + if (erofs_inode_is_data_compressed(EROFS_I(inode)->datalayout)) { int err = z_erofs_map_blocks_iter(inode, map, flags); if (map->mpage) { @@ -365,7 +365,7 @@ static int erofs_raw_access_readpages(struct file *filp, if (IS_ERR(bio)) { pr_err("%s, readahead error at page %lu of nid %llu\n", __func__, page->index, - EROFS_V(mapping->host)->nid); + EROFS_I(mapping->host)->nid); bio = NULL; } @@ -404,7 +404,7 @@ static sector_t erofs_bmap(struct address_space *mapping, sector_t block) { struct inode *inode = mapping->host; - if (EROFS_V(inode)->datalayout == EROFS_INODE_FLAT_INLINE) { + if (EROFS_I(inode)->datalayout == EROFS_INODE_FLAT_INLINE) { erofs_blk_t blks = i_size_read(inode) >> LOG_BLOCK_SIZE; if (block >> LOG_SECTORS_PER_BLOCK >= blks) diff --git a/fs/erofs/dir.c b/fs/erofs/dir.c index 6a5b43f7fb29..a032c8217071 100644 --- a/fs/erofs/dir.c +++ b/fs/erofs/dir.c @@ -47,7 +47,7 @@ static int erofs_fill_dentries(struct inode *dir, struct dir_context *ctx, /* a corrupted entry is found */ if (nameoff + de_namelen > maxsize || de_namelen > EROFS_NAME_LEN) { - errln("bogus dirent @ nid %llu", EROFS_V(dir)->nid); + errln("bogus dirent @ nid %llu", EROFS_I(dir)->nid); DBG_BUGON(1); return -EFSCORRUPTED; } @@ -85,7 +85,7 @@ static int erofs_readdir(struct file *f, struct dir_context *ctx) break; } else if (IS_ERR(dentry_page)) { errln("fail to readdir of logical block %u of nid %llu", - i, EROFS_V(dir)->nid); + i, EROFS_I(dir)->nid); err = -EFSCORRUPTED; break; } @@ -97,7 +97,7 @@ static int erofs_readdir(struct file *f, struct dir_context *ctx) if (nameoff < sizeof(struct erofs_dirent) || nameoff >= PAGE_SIZE) { errln("%s, invalid de[0].nameoff %u @ nid %llu", - __func__, nameoff, EROFS_V(dir)->nid); + __func__, nameoff, EROFS_I(dir)->nid); err = -EFSCORRUPTED; goto skip_this; } diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c index 494b35e5830a..f6dfd0271261 100644 --- a/fs/erofs/inode.c +++ b/fs/erofs/inode.c @@ -11,7 +11,7 @@ /* no locking */ static int read_inode(struct inode *inode, void *data) { - struct erofs_vnode *vi = EROFS_V(inode); + struct erofs_inode *vi = EROFS_I(inode); struct erofs_inode_compact *dic = data; struct erofs_inode_extended *die; @@ -140,7 +140,7 @@ static int read_inode(struct inode *inode, void *data) static int fill_inline_data(struct inode *inode,
[PATCH v2 17/25] erofs: kill __submit_bio()
As Christoph pointed out [1], " Why is there __submit_bio which really just obsfucates what is going on? Also why is __submit_bio using bio_set_op_attrs instead of opencode it as the comment right next to it asks you to? " Let's use submit_bio directly instead. [1] https://lore.kernel.org/r/20190830162812.ga10...@infradead.org/ Reported-by: Christoph Hellwig Signed-off-by: Gao Xiang --- fs/erofs/data.c | 21 + fs/erofs/internal.h | 7 --- fs/erofs/zdata.c| 6 -- 3 files changed, 17 insertions(+), 17 deletions(-) diff --git a/fs/erofs/data.c b/fs/erofs/data.c index 28eda71bb1a9..70b1e353756e 100644 --- a/fs/erofs/data.c +++ b/fs/erofs/data.c @@ -40,7 +40,8 @@ static inline void read_endio(struct bio *bio) static struct bio *erofs_grab_raw_bio(struct super_block *sb, erofs_blk_t blkaddr, - unsigned int nr_pages) + unsigned int nr_pages, + bool ismeta) { struct bio *bio = bio_alloc(GFP_NOIO, nr_pages); @@ -48,6 +49,11 @@ static struct bio *erofs_grab_raw_bio(struct super_block *sb, bio_set_dev(bio, sb->s_bdev); bio->bi_iter.bi_sector = (sector_t)blkaddr << LOG_SECTORS_PER_BLOCK; bio->bi_private = sb; + if (ismeta) + bio->bi_opf = REQ_OP_READ | REQ_META; + else + bio->bi_opf = REQ_OP_READ; + return bio; } @@ -69,14 +75,14 @@ struct page *erofs_get_meta_page(struct super_block *sb, erofs_blk_t blkaddr) if (!PageUptodate(page)) { struct bio *bio; - bio = erofs_grab_raw_bio(sb, blkaddr, 1); + bio = erofs_grab_raw_bio(sb, blkaddr, 1, true); if (bio_add_page(bio, page, PAGE_SIZE, 0) != PAGE_SIZE) { err = -EFAULT; goto err_out; } - __submit_bio(bio, REQ_OP_READ, REQ_META); + submit_bio(bio); lock_page(page); /* this page has been truncated by others */ @@ -201,7 +207,7 @@ static inline struct bio *erofs_read_raw_page(struct bio *bio, /* not continuous */ *last_block + 1 != current_block) { submit_bio_retry: - __submit_bio(bio, REQ_OP_READ, 0); + submit_bio(bio); bio = NULL; } @@ -271,7 +277,7 @@ static inline struct bio *erofs_read_raw_page(struct bio *bio, if (nblocks > BIO_MAX_PAGES) nblocks = BIO_MAX_PAGES; - bio = erofs_grab_raw_bio(sb, blknr, nblocks); + bio = erofs_grab_raw_bio(sb, blknr, nblocks, false); } err = bio_add_page(bio, page, PAGE_SIZE, 0); @@ -302,8 +308,7 @@ static inline struct bio *erofs_read_raw_page(struct bio *bio, /* if updated manually, continuous pages has a gap */ if (bio) submit_bio_out: - __submit_bio(bio, REQ_OP_READ, 0); - + submit_bio(bio); return err ? ERR_PTR(err) : NULL; } @@ -367,7 +372,7 @@ static int erofs_raw_access_readpages(struct file *filp, /* the rare case (end in gaps) */ if (bio) - __submit_bio(bio, REQ_OP_READ, 0); + submit_bio(bio); return 0; } diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h index 90c62fb5f80d..13c8d841c43a 100644 --- a/fs/erofs/internal.h +++ b/fs/erofs/internal.h @@ -409,13 +409,6 @@ static inline int z_erofs_map_blocks_iter(struct inode *inode, #endif /* !CONFIG_EROFS_FS_ZIP */ /* data.c */ -static inline void __submit_bio(struct bio *bio, unsigned int op, - unsigned int op_flags) -{ - bio_set_op_attrs(bio, op, op_flags); - submit_bio(bio); -} - struct page *erofs_get_meta_page(struct super_block *sb, erofs_blk_t blkaddr); int erofs_map_blocks(struct inode *, struct erofs_map_blocks *, int); diff --git a/fs/erofs/zdata.c b/fs/erofs/zdata.c index 21ade322cc81..3010fa3d1ac3 100644 --- a/fs/erofs/zdata.c +++ b/fs/erofs/zdata.c @@ -1258,7 +1258,7 @@ static bool z_erofs_vle_submit_all(struct super_block *sb, if (bio && force_submit) { submit_bio_retry: - __submit_bio(bio, REQ_OP_READ, 0); + submit_bio(bio); bio = NULL; } @@ -1270,6 +1270,8 @@ static bool z_erofs_vle_submit_all(struct super_block *sb, bio->bi_iter.bi_sector = (sector_t)(first_index + i) << LOG_SECTORS_PER_BLOCK; bio->bi_private = bi_private; + bio->bi_opf = REQ_OP_READ; + ++nr_bios; } @@ -1290,7 +1292,7 @@ static bool z_erofs_vle_submit_all(struct super_block *sb, } while
[PATCH v2 24/25] erofs: always use iget5_locked
As Christoph said [1] [2], "Just use the slightly more complicated 32-bit version everywhere so that you have a single actually tested code path. And then remove this helper. " [1] https://lore.kernel.org/r/20190829102426.ge20...@infradead.org/ [2] https://lore.kernel.org/r/20190902125320.ga16...@infradead.org/ Reported-by: Christoph Hellwig Signed-off-by: Gao Xiang --- fs/erofs/inode.c | 7 --- 1 file changed, 7 deletions(-) diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c index a0cec3c754cd..3350ab65d892 100644 --- a/fs/erofs/inode.c +++ b/fs/erofs/inode.c @@ -243,7 +243,6 @@ static int erofs_fill_inode(struct inode *inode, int isdir) * erofs nid is 64bits, but i_ino is 'unsigned long', therefore * we should do more for 32-bit platform to find the right inode. */ -#if BITS_PER_LONG == 32 static int erofs_ilookup_test_actor(struct inode *inode, void *opaque) { const erofs_nid_t nid = *(erofs_nid_t *)opaque; @@ -258,20 +257,14 @@ static int erofs_iget_set_actor(struct inode *inode, void *opaque) inode->i_ino = erofs_inode_hash(nid); return 0; } -#endif static inline struct inode *erofs_iget_locked(struct super_block *sb, erofs_nid_t nid) { const unsigned long hashval = erofs_inode_hash(nid); -#if BITS_PER_LONG >= 64 - /* it is safe to use iget_locked for >= 64-bit platform */ - return iget_locked(sb, hashval); -#else return iget5_locked(sb, hashval, erofs_ilookup_test_actor, erofs_iget_set_actor, ); -#endif } struct inode *erofs_iget(struct super_block *sb, -- 2.17.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 23/25] erofs: use read_mapping_page instead of sb_bread
As Christoph said [1], "This seems to be your only direct use of buffer heads, which while not deprecated are a bit of an ugly step child. So if you can easily avoid creating a buffer_head dependency in a new filesystem I think you should avoid it. " [1] https://lore.kernel.org/r/20190902125109.ga9...@infradead.org/ Reported-by: Christoph Hellwig Signed-off-by: Gao Xiang --- fs/erofs/super.c | 15 +-- 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/fs/erofs/super.c b/fs/erofs/super.c index 1d9880195ef0..caf9a95173b0 100644 --- a/fs/erofs/super.c +++ b/fs/erofs/super.c @@ -98,20 +98,22 @@ static bool check_layout_compatibility(struct super_block *sb, static int erofs_read_superblock(struct super_block *sb) { struct erofs_sb_info *sbi; - struct buffer_head *bh; + struct page *page; struct erofs_super_block *dsb; unsigned int blkszbits; + void *data; int ret; - bh = sb_bread(sb, 0); - - if (!bh) { + page = read_mapping_page(sb->s_bdev->bd_inode->i_mapping, 0, NULL); + if (!page) { erofs_err(sb, "cannot read erofs superblock"); return -EIO; } sbi = EROFS_SB(sb); - dsb = (struct erofs_super_block *)(bh->b_data + EROFS_SUPER_OFFSET); + + data = kmap_atomic(page); + dsb = (struct erofs_super_block *)(data + EROFS_SUPER_OFFSET); ret = -EINVAL; if (le32_to_cpu(dsb->magic) != EROFS_SUPER_MAGIC_V1) { @@ -153,7 +155,8 @@ static int erofs_read_superblock(struct super_block *sb) } ret = 0; out: - brelse(bh); + kunmap_atomic(data); + put_page(page); return ret; } -- 2.17.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 19/25] erofs: kill all erofs specific fault injection
As Christoph suggested [1], "Please just use plain kmalloc everywhere and let the normal kernel error injection code take care of injeting any errors." [1] https://lore.kernel.org/r/20190829102426.ge20...@infradead.org/ Reported-by: Christoph Hellwig Signed-off-by: Gao Xiang --- Documentation/filesystems/erofs.txt | 5 --- fs/erofs/Kconfig| 7 --- fs/erofs/data.c | 7 --- fs/erofs/inode.c| 3 +- fs/erofs/internal.h | 65 --- fs/erofs/super.c| 70 - fs/erofs/zdata.c| 8 +--- 7 files changed, 2 insertions(+), 163 deletions(-) diff --git a/Documentation/filesystems/erofs.txt b/Documentation/filesystems/erofs.txt index 38aa9126ec98..c3b5f603b2b6 100644 --- a/Documentation/filesystems/erofs.txt +++ b/Documentation/filesystems/erofs.txt @@ -52,11 +52,6 @@ linux-erofs mailing list: Mount options = -fault_injection=%d Enable fault injection in all supported types with - specified injection rate. Supported injection type: - Type_NameType_Value - FAULT_KMALLOC0x1 - FAULT_READ_IO0x2 (no)user_xattr Setup Extended User Attributes. Note: xattr is enabled by default if CONFIG_EROFS_FS_XATTR is selected. (no)aclSetup POSIX Access Control List. Note: acl is enabled diff --git a/fs/erofs/Kconfig b/fs/erofs/Kconfig index 16316d1adca3..9d634d3a1845 100644 --- a/fs/erofs/Kconfig +++ b/fs/erofs/Kconfig @@ -27,13 +27,6 @@ config EROFS_FS_DEBUG For daily use, say N. -config EROFS_FAULT_INJECTION - bool "EROFS fault injection facility" - depends on EROFS_FS - help - Test EROFS to inject faults such as ENOMEM, EIO, and so on. - If unsure, say N. - config EROFS_FS_XATTR bool "EROFS extended attributes" depends on EROFS_FS diff --git a/fs/erofs/data.c b/fs/erofs/data.c index 3ce87a88452a..b5f5b8592d14 100644 --- a/fs/erofs/data.c +++ b/fs/erofs/data.c @@ -11,16 +11,10 @@ static void erofs_readendio(struct bio *bio) { - struct super_block *const sb = bio->bi_private; struct bio_vec *bvec; blk_status_t err = bio->bi_status; struct bvec_iter_all iter_all; - if (time_to_inject(EROFS_SB(sb), FAULT_READ_IO)) { - erofs_show_injection_info(FAULT_READ_IO); - err = BLK_STS_IOERR; - } - bio_for_each_segment_all(bvec, bio, iter_all) { struct page *page = bvec->bv_page; @@ -48,7 +42,6 @@ static struct bio *erofs_grab_raw_bio(struct super_block *sb, bio->bi_end_io = erofs_readendio; bio_set_dev(bio, sb->s_bdev); bio->bi_iter.bi_sector = (sector_t)blkaddr << LOG_SECTORS_PER_BLOCK; - bio->bi_private = sb; if (ismeta) bio->bi_opf = REQ_OP_READ | REQ_META; else diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c index 384905e0677c..8e53765a532c 100644 --- a/fs/erofs/inode.c +++ b/fs/erofs/inode.c @@ -131,7 +131,6 @@ static int erofs_fill_symlink(struct inode *inode, void *data, unsigned int m_pofs) { struct erofs_inode *vi = EROFS_I(inode); - struct erofs_sb_info *sbi = EROFS_I_SB(inode); char *lnk; /* if it cannot be handled with fast symlink scheme */ @@ -141,7 +140,7 @@ static int erofs_fill_symlink(struct inode *inode, void *data, return 0; } - lnk = erofs_kmalloc(sbi, inode->i_size + 1, GFP_KERNEL); + lnk = kmalloc(inode->i_size + 1, GFP_KERNEL); if (!lnk) return -ENOMEM; diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h index 881eb2ee18b5..d659c1941f93 100644 --- a/fs/erofs/internal.h +++ b/fs/erofs/internal.h @@ -32,23 +32,6 @@ #define DBG_BUGON(x)((void)(x)) #endif /* !CONFIG_EROFS_FS_DEBUG */ -enum { - FAULT_KMALLOC, - FAULT_READ_IO, - FAULT_MAX, -}; - -#ifdef CONFIG_EROFS_FAULT_INJECTION -extern const char *erofs_fault_name[FAULT_MAX]; -#define IS_FAULT_SET(fi, type) ((fi)->inject_type & (1 << (type))) - -struct erofs_fault_info { - atomic_t inject_ops; - unsigned int inject_rate; - unsigned int inject_type; -}; -#endif /* CONFIG_EROFS_FAULT_INJECTION */ - /* EROFS_SUPER_MAGIC_V1 to represent the whole file system */ #define EROFS_SUPER_MAGIC EROFS_SUPER_MAGIC_V1 @@ -99,62 +82,14 @@ struct erofs_sb_info { u32 feature_incompat; unsigned int mount_opt; - -#ifdef CONFIG_EROFS_FAULT_INJECTION - struct erofs_fault_info fault_info; /* For fault injection */ -#endif }; -#ifdef CONFIG_EROFS_FAULT_INJECTION -#define erofs_show_injection_info(
[PATCH v2 15/25] erofs: localize erofs_grab_bio()
As Christoph pointed out [1], "erofs_grab_bio tries to handle a bio_alloc failure, except that the function will not actually fail due the mempool backing it." Sorry about useless code, fix it now and localize erofs_grab_bio [2]. [1] https://lore.kernel.org/r/20190830162812.ga10...@infradead.org/ [2] https://lore.kernel.org/r/20190902122016.gl15...@infradead.org/ Reported-by: Christoph Hellwig Signed-off-by: Gao Xiang --- fs/erofs/data.c | 28 +++- fs/erofs/internal.h | 29 - fs/erofs/zdata.c| 10 +++--- 3 files changed, 22 insertions(+), 45 deletions(-) diff --git a/fs/erofs/data.c b/fs/erofs/data.c index be11b5ea9d2e..0136ea117934 100644 --- a/fs/erofs/data.c +++ b/fs/erofs/data.c @@ -38,6 +38,19 @@ static inline void read_endio(struct bio *bio) bio_put(bio); } +static struct bio *erofs_grab_raw_bio(struct super_block *sb, + erofs_blk_t blkaddr, + unsigned int nr_pages) +{ + struct bio *bio = bio_alloc(GFP_NOIO, nr_pages); + + bio->bi_end_io = read_endio; + bio_set_dev(bio, sb->s_bdev); + bio->bi_iter.bi_sector = (sector_t)blkaddr << LOG_SECTORS_PER_BLOCK; + bio->bi_private = sb; + return bio; +} + /* prio -- true is used for dir */ struct page *__erofs_get_meta_page(struct super_block *sb, erofs_blk_t blkaddr, bool prio, bool nofail) @@ -62,12 +75,7 @@ struct page *__erofs_get_meta_page(struct super_block *sb, if (!PageUptodate(page)) { struct bio *bio; - bio = erofs_grab_bio(sb, blkaddr, 1, sb, read_endio, nofail); - if (IS_ERR(bio)) { - DBG_BUGON(nofail); - err = PTR_ERR(bio); - goto err_out; - } + bio = erofs_grab_raw_bio(sb, blkaddr, 1); if (bio_add_page(bio, page, PAGE_SIZE, 0) != PAGE_SIZE) { err = -EFAULT; @@ -276,13 +284,7 @@ static inline struct bio *erofs_read_raw_page(struct bio *bio, if (nblocks > BIO_MAX_PAGES) nblocks = BIO_MAX_PAGES; - bio = erofs_grab_bio(sb, blknr, nblocks, sb, -read_endio, false); - if (IS_ERR(bio)) { - err = PTR_ERR(bio); - bio = NULL; - goto err_out; - } + bio = erofs_grab_raw_bio(sb, blknr, nblocks); } err = bio_add_page(bio, page, PAGE_SIZE, 0); diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h index cc1ea98c5c89..000ea92b36a3 100644 --- a/fs/erofs/internal.h +++ b/fs/erofs/internal.h @@ -411,35 +411,6 @@ static inline int z_erofs_map_blocks_iter(struct inode *inode, #endif /* !CONFIG_EROFS_FS_ZIP */ /* data.c */ -static inline struct bio *erofs_grab_bio(struct super_block *sb, -erofs_blk_t blkaddr, -unsigned int nr_pages, -void *bi_private, bio_end_io_t endio, -bool nofail) -{ - const gfp_t gfp = GFP_NOIO; - struct bio *bio; - - do { - if (nr_pages == 1) { - bio = bio_alloc(gfp | (nofail ? __GFP_NOFAIL : 0), 1); - if (!bio) { - DBG_BUGON(nofail); - return ERR_PTR(-ENOMEM); - } - break; - } - bio = bio_alloc(gfp, nr_pages); - nr_pages /= 2; - } while (!bio); - - bio->bi_end_io = endio; - bio_set_dev(bio, sb->s_bdev); - bio->bi_iter.bi_sector = (sector_t)blkaddr << LOG_SECTORS_PER_BLOCK; - bio->bi_private = bi_private; - return bio; -} - static inline void __submit_bio(struct bio *bio, unsigned int op, unsigned int op_flags) { diff --git a/fs/erofs/zdata.c b/fs/erofs/zdata.c index f06a2fad7af2..21ade322cc81 100644 --- a/fs/erofs/zdata.c +++ b/fs/erofs/zdata.c @@ -1263,9 +1263,13 @@ static bool z_erofs_vle_submit_all(struct super_block *sb, } if (!bio) { - bio = erofs_grab_bio(sb, first_index + i, -BIO_MAX_PAGES, bi_private, -z_erofs_vle_read_endio, true); + bio = bio_alloc(GFP_NOIO, BIO_MAX_PAGES); + + bio->bi_end_io = z_erofs_vle_read_endio; + bio_set_dev(bio, sb->s_bdev); + bio->bi_iter.bi_sector = (sector_t)(first_index + i) << + LOG_SECTORS
[PATCH v2 21/25] erofs: save one level of indentation
As Christoph said [1], ".. and save one level of indentation." [1] https://lore.kernel.org/r/20190829102426.ge20...@infradead.org/ Reported-by: Christoph Hellwig Signed-off-by: Gao Xiang --- fs/erofs/inode.c | 65 1 file changed, 33 insertions(+), 32 deletions(-) diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c index 8e53765a532c..5a6d3282fefb 100644 --- a/fs/erofs/inode.c +++ b/fs/erofs/inode.c @@ -193,41 +193,42 @@ static int erofs_fill_inode(struct inode *inode, int isdir) data = page_address(page); err = erofs_read_inode(inode, data + ofs); - if (!err) { - /* setup the new inode */ - switch (inode->i_mode & S_IFMT) { - case S_IFREG: - inode->i_op = _generic_iops; - inode->i_fop = _ro_fops; - break; - case S_IFDIR: - inode->i_op = _dir_iops; - inode->i_fop = _dir_fops; - break; - case S_IFLNK: - err = erofs_fill_symlink(inode, data, ofs); - if (err) - goto out_unlock; - inode_nohighmem(inode); - break; - case S_IFCHR: - case S_IFBLK: - case S_IFIFO: - case S_IFSOCK: - inode->i_op = _generic_iops; - init_special_inode(inode, inode->i_mode, inode->i_rdev); - goto out_unlock; - default: - err = -EFSCORRUPTED; + if (err) + goto out_unlock; + + /* setup the new inode */ + switch (inode->i_mode & S_IFMT) { + case S_IFREG: + inode->i_op = _generic_iops; + inode->i_fop = _ro_fops; + break; + case S_IFDIR: + inode->i_op = _dir_iops; + inode->i_fop = _dir_fops; + break; + case S_IFLNK: + err = erofs_fill_symlink(inode, data, ofs); + if (err) goto out_unlock; - } + inode_nohighmem(inode); + break; + case S_IFCHR: + case S_IFBLK: + case S_IFIFO: + case S_IFSOCK: + inode->i_op = _generic_iops; + init_special_inode(inode, inode->i_mode, inode->i_rdev); + goto out_unlock; + default: + err = -EFSCORRUPTED; + goto out_unlock; + } - if (erofs_inode_is_data_compressed(vi->datalayout)) { - err = z_erofs_fill_inode(inode); - goto out_unlock; - } - inode->i_mapping->a_ops = _raw_access_aops; + if (erofs_inode_is_data_compressed(vi->datalayout)) { + err = z_erofs_fill_inode(inode); + goto out_unlock; } + inode->i_mapping->a_ops = _raw_access_aops; out_unlock: unlock_page(page); -- 2.17.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 01/25] erofs: remove all the byte offset comments
As Christoph suggested [1], "Please remove all the byte offset comments. that is something that can easily be checked with gdb or pahole." [1] https://lore.kernel.org/r/20190829095954.gb20...@infradead.org/ Reported-by: Christoph Hellwig Signed-off-by: Gao Xiang --- fs/erofs/erofs_fs.h | 105 +++- 1 file changed, 54 insertions(+), 51 deletions(-) diff --git a/fs/erofs/erofs_fs.h b/fs/erofs/erofs_fs.h index afa7d45ca958..49335fff9d65 100644 --- a/fs/erofs/erofs_fs.h +++ b/fs/erofs/erofs_fs.h @@ -17,27 +17,28 @@ #define EROFS_REQUIREMENT_LZ4_0PADDING 0x0001 #define EROFS_ALL_REQUIREMENTS EROFS_REQUIREMENT_LZ4_0PADDING +/* 128-byte erofs on-disk super block */ struct erofs_super_block { -/* 0 */__le32 magic; /* in the little endian */ -/* 4 */__le32 checksum;/* crc32c(super_block) */ -/* 8 */__le32 features;/* (aka. feature_compat) */ -/* 12 */__u8 blkszbits; /* support block_size == PAGE_SIZE only */ -/* 13 */__u8 reserved; - -/* 14 */__le16 root_nid; -/* 16 */__le64 inos;/* total valid ino # (== f_files - f_favail) */ - -/* 24 */__le64 build_time; /* inode v1 time derivation */ -/* 32 */__le32 build_time_nsec; -/* 36 */__le32 blocks; /* used for statfs */ -/* 40 */__le32 meta_blkaddr; -/* 44 */__le32 xattr_blkaddr; -/* 48 */__u8 uuid[16]; /* 128-bit uuid for volume */ -/* 64 */__u8 volume_name[16]; /* volume name */ -/* 80 */__le32 requirements;/* (aka. feature_incompat) */ - -/* 84 */__u8 reserved2[44]; -} __packed; /* 128 bytes */ + __le32 magic; /* file system magic number */ + __le32 checksum;/* crc32c(super_block) */ + __le32 features;/* (aka. feature_compat) */ + __u8 blkszbits; /* support block_size == PAGE_SIZE only */ + __u8 reserved; + + __le16 root_nid;/* nid of root directory */ + __le64 inos;/* total valid ino # (== f_files - f_favail) */ + + __le64 build_time; /* inode v1 time derivation */ + __le32 build_time_nsec; /* inode v1 time derivation in nano scale */ + __le32 blocks; /* used for statfs */ + __le32 meta_blkaddr;/* start block address of metadata area */ + __le32 xattr_blkaddr; /* start block address of shared xattr area */ + __u8 uuid[16]; /* 128-bit uuid for volume */ + __u8 volume_name[16]; /* volume name */ + __le32 requirements;/* (aka. feature_incompat) */ + + __u8 reserved2[44]; +} __packed; /* * erofs inode data mapping: @@ -73,16 +74,17 @@ static inline bool erofs_inode_is_data_compressed(unsigned int datamode) #define EROFS_I_VERSION_BIT 0 #define EROFS_I_DATA_MAPPING_BIT1 +/* 32-byte reduced form of an ondisk inode */ struct erofs_inode_v1 { -/* 0 */__le16 i_advise; + __le16 i_advise;/* inode hints */ /* 1 header + n-1 * 4 bytes inline xattr to keep continuity */ -/* 2 */__le16 i_xattr_icount; -/* 4 */__le16 i_mode; -/* 6 */__le16 i_nlink; -/* 8 */__le32 i_size; -/* 12 */__le32 i_reserved; -/* 16 */union { + __le16 i_xattr_icount; + __le16 i_mode; + __le16 i_nlink; + __le32 i_size; + __le32 i_reserved; + union { /* file total compressed blocks for data mapping 1 */ __le32 compressed_blocks; __le32 raw_blkaddr; @@ -90,10 +92,10 @@ struct erofs_inode_v1 { /* for device files, used to indicate old/new device # */ __le32 rdev; } i_u __packed; -/* 20 */__le32 i_ino; /* only used for 32-bit stat compatibility */ -/* 24 */__le16 i_uid; -/* 26 */__le16 i_gid; -/* 28 */__le32 i_reserved2; + __le32 i_ino; /* only used for 32-bit stat compatibility */ + __le16 i_uid; + __le16 i_gid; + __le32 i_reserved2; } __packed; /* 32 bytes on-disk inode */ @@ -101,15 +103,16 @@ struct erofs_inode_v1 { /* 64 bytes on-disk inode */ #define EROFS_INODE_LAYOUT_V2 1 +/* 64-byte complete form of an ondisk inode */ struct erofs_inode_v2 { -/* 0 */__le16 i_advise; + __le16 i_advise;/* inode hints */ /* 1 header + n-1 * 4 bytes inline xattr to keep continuity */ -/* 2 */__le16 i_xattr_icount; -/* 4 */__le16 i_mode; -/* 6 */__le16 i_reserved; -/* 8 */__le64 i_size; -/* 16 */union { + __le16 i_xattr_icount; + __le16 i_mode; + __le16 i_reserved; + __le64 i_size; + union { /* file total compressed blocks for data mapping 1 */ __le32 compressed_blocks; __le32 raw_blkaddr; @@ -119,15 +122,15 @@ struct erofs_inode_v2 { } i_u __packed; /* only used for 32-bit stat compatibility */ -/* 20 */__le32 i_ino; - -/* 24 */__le32 i_uid; -/* 28 */__le32 i_gid; -/* 32 */__le64 i_ctime; -/* 40 */__le32 i_ctime_nsec; -/* 44 */__le
[PATCH v2 14/25] erofs: kill verbose debug info in erofs_fill_super
As Christoph said [1], "That is some very verbose debug info. We usually don't add that and let people trace the function instead. " [1] https://lore.kernel.org/r/20190829101545.gc20...@infradead.org/ Reported-by: Christoph Hellwig Signed-off-by: Gao Xiang --- fs/erofs/super.c | 9 ++--- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/fs/erofs/super.c b/fs/erofs/super.c index 63cb17a4073b..b64d69f18270 100644 --- a/fs/erofs/super.c +++ b/fs/erofs/super.c @@ -384,9 +384,6 @@ static int erofs_fill_super(struct super_block *sb, void *data, int silent) struct erofs_sb_info *sbi; int err; - infoln("fill_super, device -> %s", sb->s_id); - infoln("options -> %s", (char *)data); - sb->s_magic = EROFS_SUPER_MAGIC; if (!sb_set_blocksize(sb, EROFS_BLKSIZ)) { @@ -419,9 +416,6 @@ static int erofs_fill_super(struct super_block *sb, void *data, int silent) if (err) return err; - if (!silent) - infoln("root inode @ nid %llu", ROOT_NID(sbi)); - if (test_opt(sbi, POSIX_ACL)) sb->s_flags |= SB_POSIXACL; else @@ -454,7 +448,8 @@ static int erofs_fill_super(struct super_block *sb, void *data, int silent) return err; if (!silent) - infoln("mounted on %s with opts: %s.", sb->s_id, (char *)data); + infoln("mounted on %s with opts: %s, root inode @ nid %llu.", + sb->s_id, (char *)data, ROOT_NID(sbi)); return 0; } -- 2.17.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 02/25] erofs: on-disk format should have explicitly assigned numbers
As Christoph suggested [1], on-disk format should have explicitly assigned numbers. [1] https://lore.kernel.org/r/20190829095954.gb20...@infradead.org/ Reported-by: Christoph Hellwig Signed-off-by: Gao Xiang --- fs/erofs/erofs_fs.h | 18 +- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/fs/erofs/erofs_fs.h b/fs/erofs/erofs_fs.h index 49335fff9d65..d1f152a3670a 100644 --- a/fs/erofs/erofs_fs.h +++ b/fs/erofs/erofs_fs.h @@ -53,10 +53,10 @@ struct erofs_super_block { * 4~7 - reserved */ enum { - EROFS_INODE_FLAT_PLAIN, - EROFS_INODE_FLAT_COMPRESSION_LEGACY, - EROFS_INODE_FLAT_INLINE, - EROFS_INODE_FLAT_COMPRESSION, + EROFS_INODE_FLAT_PLAIN = 0, + EROFS_INODE_FLAT_COMPRESSION_LEGACY = 1, + EROFS_INODE_FLAT_INLINE = 2, + EROFS_INODE_FLAT_COMPRESSION= 3, EROFS_INODE_LAYOUT_MAX }; @@ -184,7 +184,7 @@ struct erofs_xattr_entry { /* available compression algorithm types */ enum { - Z_EROFS_COMPRESSION_LZ4, + Z_EROFS_COMPRESSION_LZ4 = 0, Z_EROFS_COMPRESSION_MAX }; @@ -242,10 +242,10 @@ struct z_erofs_map_header { *(di_advise could be 0, 1 or 2) */ enum { - Z_EROFS_VLE_CLUSTER_TYPE_PLAIN, - Z_EROFS_VLE_CLUSTER_TYPE_HEAD, - Z_EROFS_VLE_CLUSTER_TYPE_NONHEAD, - Z_EROFS_VLE_CLUSTER_TYPE_RESERVED, + Z_EROFS_VLE_CLUSTER_TYPE_PLAIN = 0, + Z_EROFS_VLE_CLUSTER_TYPE_HEAD = 1, + Z_EROFS_VLE_CLUSTER_TYPE_NONHEAD= 2, + Z_EROFS_VLE_CLUSTER_TYPE_RESERVED = 3, Z_EROFS_VLE_CLUSTER_TYPE_MAX }; -- 2.17.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 10/25] erofs: update erofs_fs.h comments
As Christoph said [1] [2], update it now. [1] https://lore.kernel.org/r/20190902124521.ga22...@infradead.org/ [2] https://lore.kernel.org/r/20190902120548.gb15...@infradead.org/ Reported-by: Christoph Hellwig Signed-off-by: Gao Xiang --- fs/erofs/erofs_fs.h | 9 + 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/fs/erofs/erofs_fs.h b/fs/erofs/erofs_fs.h index 18689e916e94..b1ee5654750d 100644 --- a/fs/erofs/erofs_fs.h +++ b/fs/erofs/erofs_fs.h @@ -1,5 +1,7 @@ /* SPDX-License-Identifier: GPL-2.0-only OR Apache-2.0 */ /* + * EROFS (Enhanced ROM File System) on-disk format definition + * * Copyright (C) 2017-2018 HUAWEI, Inc. * http://www.huawei.com/ * Created by Gao Xiang @@ -7,7 +9,6 @@ #ifndef __EROFS_FS_H #define __EROFS_FS_H -/* Enhanced(Extended) ROM File System */ #define EROFS_SUPER_OFFSET 1024 /* @@ -41,7 +42,7 @@ struct erofs_super_block { }; /* - * erofs inode datalayout: + * erofs inode datalayout (i_format in on-disk inode): * 0 - inode plain without inline data A: * inode, [xattrs], ... | ... | no-holed data * 1 - inode VLE compression B (legacy): @@ -187,7 +188,7 @@ static inline unsigned int erofs_xattr_entry_size(struct erofs_xattr_entry *e) e->e_name_len + le16_to_cpu(e->e_value_size)); } -/* available compression algorithm types */ +/* available compression algorithm types (for h_algorithmtype) */ enum { Z_EROFS_COMPRESSION_LZ4 = 0, Z_EROFS_COMPRESSION_MAX @@ -222,7 +223,7 @@ struct z_erofs_map_header { #define Z_EROFS_VLE_LEGACY_HEADER_PADDING 8 /* - * Z_EROFS Variable-sized Logical Extent cluster type: + * Fixed-sized output compression ondisk Logical Extent cluster type: *0 - literal (uncompressed) cluster *1 - compressed cluster (for the head logical cluster) *2 - compressed cluster (for the other logical clusters) -- 2.17.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 11/25] erofs: update comments in inode.c
As Christoph suggested [1], update them all. [1] https://lore.kernel.org/r/20190829102426.ge20...@infradead.org/ Reported-by: Christoph Hellwig Signed-off-by: Gao Xiang --- fs/erofs/inode.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c index f6dfd0271261..a42f5fc14df9 100644 --- a/fs/erofs/inode.c +++ b/fs/erofs/inode.c @@ -147,7 +147,7 @@ static int fill_inline_data(struct inode *inode, void *data, if (vi->datalayout != EROFS_INODE_FLAT_INLINE) return 0; - /* fast symlink (following ext4) */ + /* fast symlink */ if (S_ISLNK(inode->i_mode) && inode->i_size < PAGE_SIZE) { char *lnk = erofs_kmalloc(sbi, inode->i_size + 1, GFP_KERNEL); @@ -156,7 +156,7 @@ static int fill_inline_data(struct inode *inode, void *data, m_pofs += vi->inode_isize + vi->xattr_isize; - /* inline symlink data shouldn't across page boundary as well */ + /* inline symlink data shouldn't cross page boundary as well */ if (m_pofs + inode->i_size > PAGE_SIZE) { kfree(lnk); errln("inline data cross block boundary @ nid %llu", @@ -165,7 +165,6 @@ static int fill_inline_data(struct inode *inode, void *data, return -EFSCORRUPTED; } - /* get in-page inline data */ memcpy(lnk, data + m_pofs, inode->i_size); lnk[inode->i_size] = '\0'; -- 2.17.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 13/25] erofs: use dsb instead of layout for ondisk super_block
As Christoph pointed out [1], "Why is the variable name for the on-disk subperblock layout? We usually still calls this something with sb in the name, e.g. dsb. for disksuper block. " Let's fix it. [1] https://lore.kernel.org/r/20190829101545.gc20...@infradead.org/ Signed-off-by: Gao Xiang --- fs/erofs/super.c | 35 +-- 1 file changed, 17 insertions(+), 18 deletions(-) diff --git a/fs/erofs/super.c b/fs/erofs/super.c index b8b0e35f6621..63cb17a4073b 100644 --- a/fs/erofs/super.c +++ b/fs/erofs/super.c @@ -49,9 +49,9 @@ static void free_inode(struct inode *inode) } static bool check_layout_compatibility(struct super_block *sb, - struct erofs_super_block *layout) + struct erofs_super_block *dsb) { - const unsigned int feature = le32_to_cpu(layout->feature_incompat); + const unsigned int feature = le32_to_cpu(dsb->feature_incompat); EROFS_SB(sb)->feature_incompat = feature; @@ -68,7 +68,7 @@ static int superblock_read(struct super_block *sb) { struct erofs_sb_info *sbi; struct buffer_head *bh; - struct erofs_super_block *layout; + struct erofs_super_block *dsb; unsigned int blkszbits; int ret; @@ -80,16 +80,15 @@ static int superblock_read(struct super_block *sb) } sbi = EROFS_SB(sb); - layout = (struct erofs_super_block *)((u8 *)bh->b_data -+ EROFS_SUPER_OFFSET); + dsb = (struct erofs_super_block *)(bh->b_data + EROFS_SUPER_OFFSET); ret = -EINVAL; - if (le32_to_cpu(layout->magic) != EROFS_SUPER_MAGIC_V1) { + if (le32_to_cpu(dsb->magic) != EROFS_SUPER_MAGIC_V1) { errln("cannot find valid erofs superblock"); goto out; } - blkszbits = layout->blkszbits; + blkszbits = dsb->blkszbits; /* 9(512 bytes) + LOG_SECTORS_PER_BLOCK == LOG_BLOCK_SIZE */ if (blkszbits != LOG_BLOCK_SIZE) { errln("blksize %u isn't supported on this platform", @@ -97,25 +96,25 @@ static int superblock_read(struct super_block *sb) goto out; } - if (!check_layout_compatibility(sb, layout)) + if (!check_layout_compatibility(sb, dsb)) goto out; - sbi->blocks = le32_to_cpu(layout->blocks); - sbi->meta_blkaddr = le32_to_cpu(layout->meta_blkaddr); + sbi->blocks = le32_to_cpu(dsb->blocks); + sbi->meta_blkaddr = le32_to_cpu(dsb->meta_blkaddr); #ifdef CONFIG_EROFS_FS_XATTR - sbi->xattr_blkaddr = le32_to_cpu(layout->xattr_blkaddr); + sbi->xattr_blkaddr = le32_to_cpu(dsb->xattr_blkaddr); #endif sbi->islotbits = ilog2(sizeof(struct erofs_inode_compact)); - sbi->root_nid = le16_to_cpu(layout->root_nid); - sbi->inos = le64_to_cpu(layout->inos); + sbi->root_nid = le16_to_cpu(dsb->root_nid); + sbi->inos = le64_to_cpu(dsb->inos); - sbi->build_time = le64_to_cpu(layout->build_time); - sbi->build_time_nsec = le32_to_cpu(layout->build_time_nsec); + sbi->build_time = le64_to_cpu(dsb->build_time); + sbi->build_time_nsec = le32_to_cpu(dsb->build_time_nsec); - memcpy(>s_uuid, layout->uuid, sizeof(layout->uuid)); + memcpy(>s_uuid, dsb->uuid, sizeof(dsb->uuid)); - ret = strscpy(sbi->volume_name, layout->volume_name, - sizeof(layout->volume_name)); + ret = strscpy(sbi->volume_name, dsb->volume_name, + sizeof(dsb->volume_name)); if (ret < 0) { /* -E2BIG */ errln("bad volume name without NIL terminator"); ret = -EFSCORRUPTED; -- 2.17.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 12/25] erofs: better erofs symlink stuffs
Fix as Christoph suggested [1] [2], "remove is_inode_fast_symlink and just opencode it in the few places using it" and "Please just set the ops directly instead of obsfucating that in a single caller, single line inline function. And please set it instead of the normal symlink iops in the same place where you also set those." [1] https://lore.kernel.org/r/20190830163910.gb29...@infradead.org/ [2] https://lore.kernel.org/r/20190829102426.ge20...@infradead.org/ Reported-by: Christoph Hellwig Signed-off-by: Gao Xiang --- fs/erofs/inode.c| 68 ++--- fs/erofs/internal.h | 10 --- fs/erofs/super.c| 5 ++-- 3 files changed, 29 insertions(+), 54 deletions(-) diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c index a42f5fc14df9..770f3259c862 100644 --- a/fs/erofs/inode.c +++ b/fs/erofs/inode.c @@ -127,50 +127,39 @@ static int read_inode(struct inode *inode, void *data) return -EFSCORRUPTED; } -/* - * try_lock can be required since locking order is: - * file data(fs_inode) - *meta(bd_inode) - * but the majority of the callers is "iget", - * in that case we are pretty sure no deadlock since - * no data operations exist. However I tend to - * try_lock since it takes no much overhead and - * will success immediately. - */ -static int fill_inline_data(struct inode *inode, void *data, - unsigned int m_pofs) +static int erofs_fill_symlink(struct inode *inode, void *data, + unsigned int m_pofs) { struct erofs_inode *vi = EROFS_I(inode); struct erofs_sb_info *sbi = EROFS_I_SB(inode); + char *lnk; - /* should be tail-packing data inline */ - if (vi->datalayout != EROFS_INODE_FLAT_INLINE) + /* if it cannot be handled with fast symlink scheme */ + if (vi->datalayout != EROFS_INODE_FLAT_INLINE || + inode->i_size >= PAGE_SIZE) { + inode->i_op = _symlink_iops; return 0; + } - /* fast symlink */ - if (S_ISLNK(inode->i_mode) && inode->i_size < PAGE_SIZE) { - char *lnk = erofs_kmalloc(sbi, inode->i_size + 1, GFP_KERNEL); - - if (!lnk) - return -ENOMEM; - - m_pofs += vi->inode_isize + vi->xattr_isize; + lnk = erofs_kmalloc(sbi, inode->i_size + 1, GFP_KERNEL); + if (!lnk) + return -ENOMEM; - /* inline symlink data shouldn't cross page boundary as well */ - if (m_pofs + inode->i_size > PAGE_SIZE) { - kfree(lnk); - errln("inline data cross block boundary @ nid %llu", - vi->nid); - DBG_BUGON(1); - return -EFSCORRUPTED; - } + m_pofs += vi->inode_isize + vi->xattr_isize; + /* inline symlink data shouldn't cross page boundary as well */ + if (m_pofs + inode->i_size > PAGE_SIZE) { + kfree(lnk); + errln("inline data cross block boundary @ nid %llu", + vi->nid); + DBG_BUGON(1); + return -EFSCORRUPTED; + } - memcpy(lnk, data + m_pofs, inode->i_size); - lnk[inode->i_size] = '\0'; + memcpy(lnk, data + m_pofs, inode->i_size); + lnk[inode->i_size] = '\0'; - inode->i_link = lnk; - set_inode_fast_symlink(inode); - } + inode->i_link = lnk; + inode->i_op = _fast_symlink_iops; return 0; } @@ -217,8 +206,9 @@ static int fill_inode(struct inode *inode, int isdir) inode->i_fop = _dir_fops; break; case S_IFLNK: - /* by default, page_get_link is used for symlink */ - inode->i_op = _symlink_iops; + err = erofs_fill_symlink(inode, data, ofs); + if (err) + goto out_unlock; inode_nohighmem(inode); break; case S_IFCHR: @@ -237,11 +227,7 @@ static int fill_inode(struct inode *inode, int isdir) err = z_erofs_fill_inode(inode); goto out_unlock; } - inode->i_mapping->a_ops = _raw_access_aops; - - /* fill last page if inline data is available */ - err = fill_inline_data(inode, data, ofs); } out_unlock: diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h index 10497ee07cae..cc1ea98c5c89 100644 --- a/fs/erofs/internal.h +++ b/fs/erofs/internal.h @@ -479,16 +479,6 @@ extern const struct inode_operations erofs_generic_iops; extern const struct inode_operations erofs_symlink_iops;
[PATCH v2 05/25] erofs: update erofs_inode_is_data_compressed helper
As Christoph said, "This looks like a really obsfucated way to write: return datamode == EROFS_INODE_FLAT_COMPRESSION || datamode == EROFS_INODE_FLAT_COMPRESSION_LEGACY; " Although I had my own consideration, it's the right way for now. [1] https://lore.kernel.org/r/20190829095954.gb20...@infradead.org/ Reported-by: Christoph Hellwig Signed-off-by: Gao Xiang --- fs/erofs/erofs_fs.h | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/fs/erofs/erofs_fs.h b/fs/erofs/erofs_fs.h index 59dcc2e8cb02..87d7ae82339a 100644 --- a/fs/erofs/erofs_fs.h +++ b/fs/erofs/erofs_fs.h @@ -62,9 +62,8 @@ enum { static inline bool erofs_inode_is_data_compressed(unsigned int datamode) { - if (datamode == EROFS_INODE_FLAT_COMPRESSION) - return true; - return datamode == EROFS_INODE_FLAT_COMPRESSION_LEGACY; + return datamode == EROFS_INODE_FLAT_COMPRESSION || + datamode == EROFS_INODE_FLAT_COMPRESSION_LEGACY; } /* bit definitions of inode i_advise */ -- 2.17.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 06/25] erofs: use feature_incompat rather than requirements
As Christoph said [1], "This is only cosmetic, why not stick to feature_compat and feature_incompat?" In my thought, requirements means "incompatible" instead of "feature" though. [1] https://lore.kernel.org/r/20190902125109.ga9...@infradead.org/ Reported-by: Christoph Hellwig Signed-off-by: Gao Xiang --- fs/erofs/decompressor.c | 3 ++- fs/erofs/erofs_fs.h | 12 ++-- fs/erofs/internal.h | 2 +- fs/erofs/super.c| 10 +- 4 files changed, 14 insertions(+), 13 deletions(-) diff --git a/fs/erofs/decompressor.c b/fs/erofs/decompressor.c index df349888f911..555c04730f87 100644 --- a/fs/erofs/decompressor.c +++ b/fs/erofs/decompressor.c @@ -129,7 +129,8 @@ static int lz4_decompress(struct z_erofs_decompress_req *rq, u8 *out) support_0padding = false; /* decompression inplace is only safe when 0padding is enabled */ - if (EROFS_SB(rq->sb)->requirements & EROFS_REQUIREMENT_LZ4_0PADDING) { + if (EROFS_SB(rq->sb)->feature_incompat & + EROFS_FEATURE_INCOMPAT_LZ4_0PADDING) { support_0padding = true; while (!src[inputmargin & ~PAGE_MASK]) diff --git a/fs/erofs/erofs_fs.h b/fs/erofs/erofs_fs.h index 87d7ae82339a..b2aef3bc377d 100644 --- a/fs/erofs/erofs_fs.h +++ b/fs/erofs/erofs_fs.h @@ -11,17 +11,17 @@ #define EROFS_SUPER_OFFSET 1024 /* - * Any bits that aren't in EROFS_ALL_REQUIREMENTS should be - * incompatible with this kernel version. + * Any bits that aren't in EROFS_ALL_FEATURE_INCOMPAT should + * be incompatible with this kernel version. */ -#define EROFS_REQUIREMENT_LZ4_0PADDING 0x0001 -#define EROFS_ALL_REQUIREMENTS EROFS_REQUIREMENT_LZ4_0PADDING +#define EROFS_FEATURE_INCOMPAT_LZ4_0PADDING0x0001 +#define EROFS_ALL_FEATURE_INCOMPAT EROFS_FEATURE_INCOMPAT_LZ4_0PADDING /* 128-byte erofs on-disk super block */ struct erofs_super_block { __le32 magic; /* file system magic number */ __le32 checksum;/* crc32c(super_block) */ - __le32 features;/* (aka. feature_compat) */ + __le32 feature_compat; __u8 blkszbits; /* support block_size == PAGE_SIZE only */ __u8 reserved; @@ -35,7 +35,7 @@ struct erofs_super_block { __le32 xattr_blkaddr; /* start block address of shared xattr area */ __u8 uuid[16]; /* 128-bit uuid for volume */ __u8 volume_name[16]; /* volume name */ - __le32 requirements;/* (aka. feature_incompat) */ + __le32 feature_incompat; __u8 reserved2[44]; }; diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h index 141ea424587d..7ff36f404ec3 100644 --- a/fs/erofs/internal.h +++ b/fs/erofs/internal.h @@ -96,7 +96,7 @@ struct erofs_sb_info { u8 uuid[16];/* 128-bit uuid for volume */ u8 volume_name[16]; /* volume name */ - u32 requirements; + u32 feature_incompat; unsigned int mount_opt; diff --git a/fs/erofs/super.c b/fs/erofs/super.c index 6603f0ba8905..6a7ab194783c 100644 --- a/fs/erofs/super.c +++ b/fs/erofs/super.c @@ -67,14 +67,14 @@ static void free_inode(struct inode *inode) static bool check_layout_compatibility(struct super_block *sb, struct erofs_super_block *layout) { - const unsigned int requirements = le32_to_cpu(layout->requirements); + const unsigned int feature = le32_to_cpu(layout->feature_incompat); - EROFS_SB(sb)->requirements = requirements; + EROFS_SB(sb)->feature_incompat = feature; /* check if current kernel meets all mandatory requirements */ - if (requirements & (~EROFS_ALL_REQUIREMENTS)) { - errln("unidentified requirements %x, please upgrade kernel version", - requirements & ~EROFS_ALL_REQUIREMENTS); + if (feature & (~EROFS_ALL_FEATURE_INCOMPAT)) { + errln("unidentified incompatible feature %x, please upgrade kernel version", + feature & ~EROFS_ALL_FEATURE_INCOMPAT); return false; } return true; -- 2.17.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 03/25] erofs: some macros are much more readable as a function
As Christoph suggested [1], these macros 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 --- fs/erofs/erofs_fs.h | 22 ++ fs/erofs/inode.c| 4 ++-- fs/erofs/xattr.c| 2 +- 3 files changed, 17 insertions(+), 11 deletions(-) diff --git a/fs/erofs/erofs_fs.h b/fs/erofs/erofs_fs.h index d1f152a3670a..c1220b0f26e0 100644 --- a/fs/erofs/erofs_fs.h +++ b/fs/erofs/erofs_fs.h @@ -171,16 +171,22 @@ struct erofs_xattr_entry { char e_name[0]; /* attribute name */ } __packed; -#define ondisk_xattr_ibody_size(count) ({\ - u32 __count = le16_to_cpu(count); \ - ((__count) == 0) ? 0 : \ - sizeof(struct erofs_xattr_ibody_header) + \ - sizeof(__u32) * ((__count) - 1); }) +static inline unsigned int erofs_xattr_ibody_size(__le16 i_xattr_icount) +{ + if (!i_xattr_icount) + return 0; + + return sizeof(struct erofs_xattr_ibody_header) + + sizeof(__u32) * (le16_to_cpu(i_xattr_icount) - 1); +} #define EROFS_XATTR_ALIGN(size) round_up(size, sizeof(struct erofs_xattr_entry)) -#define EROFS_XATTR_ENTRY_SIZE(entry) EROFS_XATTR_ALIGN( \ - sizeof(struct erofs_xattr_entry) + \ - (entry)->e_name_len + le16_to_cpu((entry)->e_value_size)) + +static inline unsigned int erofs_xattr_entry_size(struct erofs_xattr_entry *e) +{ + return EROFS_XATTR_ALIGN(sizeof(struct erofs_xattr_entry) + +e->e_name_len + le16_to_cpu(e->e_value_size)); +} /* available compression algorithm types */ enum { diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c index 8a0574530a0a..3fc4f764b387 100644 --- a/fs/erofs/inode.c +++ b/fs/erofs/inode.c @@ -29,7 +29,7 @@ static int read_inode(struct inode *inode, void *data) struct erofs_inode_v2 *v2 = data; vi->inode_isize = sizeof(struct erofs_inode_v2); - vi->xattr_isize = ondisk_xattr_ibody_size(v2->i_xattr_icount); + vi->xattr_isize = erofs_xattr_ibody_size(v2->i_xattr_icount); inode->i_mode = le16_to_cpu(v2->i_mode); if (S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode) || @@ -62,7 +62,7 @@ static int read_inode(struct inode *inode, void *data) struct erofs_sb_info *sbi = EROFS_SB(inode->i_sb); vi->inode_isize = sizeof(struct erofs_inode_v1); - vi->xattr_isize = ondisk_xattr_ibody_size(v1->i_xattr_icount); + vi->xattr_isize = erofs_xattr_ibody_size(v1->i_xattr_icount); inode->i_mode = le16_to_cpu(v1->i_mode); if (S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode) || diff --git a/fs/erofs/xattr.c b/fs/erofs/xattr.c index d80f61dde72f..620cbc15f4d0 100644 --- a/fs/erofs/xattr.c +++ b/fs/erofs/xattr.c @@ -231,7 +231,7 @@ static int xattr_foreach(struct xattr_iter *it, */ entry = *(struct erofs_xattr_entry *)(it->kaddr + it->ofs); if (tlimit) { - unsigned int entry_sz = EROFS_XATTR_ENTRY_SIZE(); + unsigned int entry_sz = erofs_xattr_entry_size(); /* xattr on-disk corruption: xattr entry beyond xattr_isize */ if (*tlimit < entry_sz) { -- 2.17.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 08/25] erofs: kill erofs_{init,exit}_inode_cache
As Christoph said [1] "having this function seems entirely pointless", let's kill those. filesystem function name ext2,f2fs,ext4,isofs,squashfs,cifs,... init_inodecache In addition, add a necessary "rcu_barrier()" on exit_fs(); [1] https://lore.kernel.org/r/20190829101545.gc20...@infradead.org/ Reported-by: Christoph Hellwig Signed-off-by: Gao Xiang --- fs/erofs/super.c | 31 --- 1 file changed, 12 insertions(+), 19 deletions(-) diff --git a/fs/erofs/super.c b/fs/erofs/super.c index 8d9f38d56b3b..499dc7f5d0e6 100644 --- a/fs/erofs/super.c +++ b/fs/erofs/super.c @@ -23,21 +23,6 @@ static void init_once(void *ptr) inode_init_once(>vfs_inode); } -static int __init erofs_init_inode_cache(void) -{ - erofs_inode_cachep = kmem_cache_create("erofs_inode", - sizeof(struct erofs_vnode), 0, - SLAB_RECLAIM_ACCOUNT, - init_once); - - return erofs_inode_cachep ? 0 : -ENOMEM; -} - -static void erofs_exit_inode_cache(void) -{ - kmem_cache_destroy(erofs_inode_cachep); -} - static struct inode *alloc_inode(struct super_block *sb) { struct erofs_vnode *vi = @@ -531,9 +516,14 @@ static int __init erofs_module_init(void) erofs_check_ondisk_layout_definitions(); infoln("initializing erofs " EROFS_VERSION); - err = erofs_init_inode_cache(); - if (err) + erofs_inode_cachep = kmem_cache_create("erofs_inode", + sizeof(struct erofs_vnode), 0, + SLAB_RECLAIM_ACCOUNT, + init_once); + if (!erofs_inode_cachep) { + err = -ENOMEM; goto icache_err; + } err = erofs_init_shrinker(); if (err) @@ -555,7 +545,7 @@ static int __init erofs_module_init(void) zip_err: erofs_exit_shrinker(); shrinker_err: - erofs_exit_inode_cache(); + kmem_cache_destroy(erofs_inode_cachep); icache_err: return err; } @@ -565,7 +555,10 @@ static void __exit erofs_module_exit(void) unregister_filesystem(_fs_type); z_erofs_exit_zip_subsystem(); erofs_exit_shrinker(); - erofs_exit_inode_cache(); + + /* Ensure all RCU free inodes are safe before cache is destroyed. */ + rcu_barrier(); + kmem_cache_destroy(erofs_inode_cachep); infoln("successfully finalize erofs"); } -- 2.17.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 00/25] erofs: patchset addressing Christoph's comments
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. 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(-) -- 2.17.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 04/25] erofs: kill __packed for on-disk structures
As Christoph suggested "Please don't add __packed" [1], 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/r/20190829095954.gb20...@infradead.org/ Reported-by: Christoph Hellwig Signed-off-by: Gao Xiang --- fs/erofs/erofs_fs.h | 18 +- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/fs/erofs/erofs_fs.h b/fs/erofs/erofs_fs.h index c1220b0f26e0..59dcc2e8cb02 100644 --- a/fs/erofs/erofs_fs.h +++ b/fs/erofs/erofs_fs.h @@ -38,7 +38,7 @@ struct erofs_super_block { __le32 requirements;/* (aka. feature_incompat) */ __u8 reserved2[44]; -} __packed; +}; /* * erofs inode data mapping: @@ -91,12 +91,12 @@ struct erofs_inode_v1 { /* for device files, used to indicate old/new device # */ __le32 rdev; - } i_u __packed; + } i_u; __le32 i_ino; /* only used for 32-bit stat compatibility */ __le16 i_uid; __le16 i_gid; __le32 i_reserved2; -} __packed; +}; /* 32 bytes on-disk inode */ #define EROFS_INODE_LAYOUT_V1 0 @@ -119,7 +119,7 @@ struct erofs_inode_v2 { /* for device files, used to indicate old/new device # */ __le32 rdev; - } i_u __packed; + } i_u; /* only used for 32-bit stat compatibility */ __le32 i_ino; @@ -130,7 +130,7 @@ struct erofs_inode_v2 { __le32 i_ctime_nsec; __le32 i_nlink; __u8 i_reserved2[16]; -} __packed; +}; #define EROFS_MAX_SHARED_XATTRS (128) /* h_shared_count between 129 ... 255 are special # */ @@ -152,7 +152,7 @@ struct erofs_xattr_ibody_header { __u8 h_shared_count; __u8 h_reserved2[7]; __le32 h_shared_xattrs[0]; /* shared xattr id array */ -} __packed; +}; /* Name indexes */ #define EROFS_XATTR_INDEX_USER 1 @@ -169,7 +169,7 @@ struct erofs_xattr_entry { __le16 e_value_size;/* size of attribute value */ /* followed by e_name and e_value */ char e_name[0]; /* attribute name */ -} __packed; +}; static inline unsigned int erofs_xattr_ibody_size(__le16 i_xattr_icount) { @@ -273,8 +273,8 @@ struct z_erofs_vle_decompressed_index { * [1] - pointing to the tail cluster */ __le16 delta[2]; - } di_u __packed; -} __packed; + } di_u; +}; #define Z_EROFS_VLE_LEGACY_INDEX_ALIGN(size) \ (round_up(size, sizeof(struct z_erofs_vle_decompressed_index)) + \ -- 2.17.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 00/21] erofs: patchset addressing Christoph's comments
Hi Christoph, On Tue, Sep 03, 2019 at 08:37:04AM -0700, Christoph Hellwig wrote: > On Tue, Sep 03, 2019 at 04:17:49PM +0800, Gao Xiang wrote: > > I implement a prelimitary version, but I have no idea it is a really > > cleanup for now. > > The fact that this has to guess the block device address_space > implementation is indeed pretty ugly. I'd much prefer to just use > read_cache_page_gfp, and live with the fact that this allocates > bufferheads behind you for now. I'll try to speed up my attempts to > get rid of the buffer heads on the block device mapping instead. Fully agree with your point. Let me use read_cache_page_gfp instead for now, hoping block device quickly avoiding from buffer_heads... Thanks, Gao Xiang ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 00/21] erofs: patchset addressing Christoph's comments
Hi Christoph, On Mon, Sep 02, 2019 at 11:58:03PM -0700, Christoph Hellwig wrote: > On Mon, Sep 02, 2019 at 11:50:38PM +0800, Gao Xiang wrote: > > > > You means killing erofs_get_meta_page or avoid erofs_read_raw_page? > > > > > > > > - For killing erofs_get_meta_page, here is the current > > > > erofs_get_meta_page: > > > > > > > I think it is simple enough. read_cache_page need write a similar > > > > filler, or read_cache_page_gfp will call .readpage, and then > > > > introduce buffer_heads, that is what I'd like to avoid now (no need > > > > these > > > > bd_inode buffer_heads in memory...) > > > > > > If using read_cache_page_gfp and ->readpage works, please do. The > > > fact that the block device inode uses buffer heads is an implementation > > > detail that might not last very long and should be invisible to you. > > > It also means you can get rid of a lot of code that you don't have > > > to maintain and others don't have to update for global API changes. > > > > I care about those useless buffer_heads in memory for our products... > > > > Since we are nobh filesystem (a little request, could I use it > > after buffer_heads are fully avoided, I have no idea why I need > > those buffer_heads in memory But I think bd_inode is good > > for caching metadata...) > > Then please use read_cache_page with iomap_readpage(s), and write > comment explaining why your are not using read_cache_page_gfp. I implement a prelimitary version, but I have no idea it is a really cleanup for now. >From 001e3e64c81e4ced0d22b147e6abf90060e704b5 Mon Sep 17 00:00:00 2001 From: Gao Xiang Date: Tue, 3 Sep 2019 16:13:00 +0800 Subject: [PATCH] erofs: use iomap_readpage for erofs_get_meta_page Signed-off-by: Gao Xiang --- fs/erofs/Kconfig | 1 + fs/erofs/data.c | 91 ++-- 2 files changed, 51 insertions(+), 41 deletions(-) diff --git a/fs/erofs/Kconfig b/fs/erofs/Kconfig index 9d634d3a1845..c9eeb0bf4737 100644 --- a/fs/erofs/Kconfig +++ b/fs/erofs/Kconfig @@ -3,6 +3,7 @@ config EROFS_FS tristate "EROFS filesystem support" depends on BLOCK + select FS_IOMAP help EROFS (Enhanced Read-Only File System) is a lightweight read-only file system with modern designs (eg. page-sized diff --git a/fs/erofs/data.c b/fs/erofs/data.c index 3881c0689134..34c6e05fab71 100644 --- a/fs/erofs/data.c +++ b/fs/erofs/data.c @@ -5,6 +5,9 @@ * Created by Gao Xiang */ #include "internal.h" +#include +#include +#include #include #include @@ -51,54 +54,60 @@ static struct bio *erofs_grab_raw_bio(struct super_block *sb, return bio; } -struct page *erofs_get_meta_page(struct super_block *sb, erofs_blk_t blkaddr) +static int erofs_meta_iomap_begin(struct inode *inode, loff_t pos, + loff_t length, unsigned int flags, + struct iomap *iomap) { - struct inode *const bd_inode = sb->s_bdev->bd_inode; - struct address_space *const mapping = bd_inode->i_mapping; - /* prefer retrying in the allocator to blindly looping below */ - const gfp_t gfp = mapping_gfp_constraint(mapping, ~__GFP_FS); - struct page *page; - int err; - -repeat: - page = find_or_create_page(mapping, blkaddr, gfp); - if (!page) - return ERR_PTR(-ENOMEM); - - DBG_BUGON(!PageLocked(page)); - - if (!PageUptodate(page)) { - struct bio *bio; + const unsigned int blkbits = inode->i_blkbits; + + iomap->flags = 0; + iomap->bdev = I_BDEV(inode); + iomap->offset = round_down(pos, 1 << blkbits); + iomap->addr = iomap->offset; + iomap->length = round_up(length, 1 << blkbits); + iomap->type = IOMAP_MAPPED; + return 0; +} - bio = erofs_grab_raw_bio(sb, blkaddr, 1, true); +static const struct iomap_ops erofs_meta_iomap_ops = { + .iomap_begin = erofs_meta_iomap_begin, +}; - if (bio_add_page(bio, page, PAGE_SIZE, 0) != PAGE_SIZE) { - err = -EFAULT; - goto err_out; - } +static int +erofs_meta_get_block(struct inode *inode, sector_t iblock, +struct buffer_head *bh, int create) +{ + bh->b_bdev = I_BDEV(inode); + bh->b_blocknr = iblock; + set_buffer_mapped(bh); + return 0; +} - submit_bio(bio); - lock_page(page); +static int erofs_read_meta_page(void *file, struct page *page) +{ + /* in case of getting some pages with buffer_heads */ + if (i_blocksize(page->mapping->
Re: [PATCH 00/21] erofs: patchset addressing Christoph's comments
Hi Christoph, On Mon, Sep 02, 2019 at 08:23:23AM -0700, Christoph Hellwig wrote: > On Mon, Sep 02, 2019 at 10:24:52PM +0800, Gao Xiang wrote: > > > code quality stuff. We're not addressing the issues with large amounts > > > of functionality duplicating VFS helpers. > > > > You means killing erofs_get_meta_page or avoid erofs_read_raw_page? > > > > - For killing erofs_get_meta_page, here is the current erofs_get_meta_page: > > > I think it is simple enough. read_cache_page need write a similar > > filler, or read_cache_page_gfp will call .readpage, and then > > introduce buffer_heads, that is what I'd like to avoid now (no need these > > bd_inode buffer_heads in memory...) > > If using read_cache_page_gfp and ->readpage works, please do. The > fact that the block device inode uses buffer heads is an implementation > detail that might not last very long and should be invisible to you. > It also means you can get rid of a lot of code that you don't have > to maintain and others don't have to update for global API changes. I care about those useless buffer_heads in memory for our products... Since we are nobh filesystem (a little request, could I use it after buffer_heads are fully avoided, I have no idea why I need those buffer_heads in memory But I think bd_inode is good for caching metadata...) > > > - For erofs_read_raw_page, it can be avoided after iomap tail-end packing > >feature is done... If we remove it now, it will make EROFS broken. > >It is no urgent and Chao will focus on iomap tail-end packing feature. > > Ok. I wish we would have just sorted this out beforehand, which we > could have trivially done without all that staging mess. Firstly, I'd like to introduce EROFS as a self-contained filesystem to introduce new fixed-sized output compression to upstream and promote it... And then we can do many improvements for EROFS in parallel... (if we introduce EROFS and touch many core modules like iomap, mm readahead code or modify LZ4 code at once... It could be more careful... Let's improve it step-by-step... We are a dedicated team if the Linux community needs us, we will still here... It will be actively maintained.) Thanks, Gao Xiang ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v6 03/24] erofs: add super block operations
Hi Christoph, On Mon, Sep 02, 2019 at 08:19:10AM -0700, Christoph Hellwig wrote: > On Mon, Sep 02, 2019 at 10:43:04PM +0800, Gao Xiang wrote: > > Hi Christoph, > > > > ... > > > > 24 __le32 features;/* (aka. feature_compat) */ > > > > ... > > > > 38 __le32 requirements;/* (aka. feature_incompat) */ > > > > ... > > > > 41 }; > > > > > > This is only cosmetic, why not stick to feature_compat and > > > feature_incompat? > > > > Okay, will fix. (however, in my mind, I'm some confused why > > "features" could be incompatible...) > > The feature is incompatible if it requires changes to the driver. > An easy to understand historic example is that ext3 originally did not > have the file types in the directory entry. Adding them means old > file system drivers can not read a file system with this new feature, > so an incompat flag has to be added. Got it. > > > > > > > + memcpy(>s_uuid, layout->uuid, sizeof(layout->uuid)); > > > > > > + memcpy(sbi->volume_name, layout->volume_name, > > > > > > + sizeof(layout->volume_name)); > > > > > > > > > > s_uuid should preferably be a uuid_t (assuming it is a real BE uuid, > > > > > if it is le it should be a guid_t). > > > > > > > > For this case, I have no idea how to deal with... > > > > I have little knowledge about this uuid stuff, so I just copied > > > > from f2fs... (Could be no urgent of this field...) > > > > > > Who fills out this field in the on-disk format and how? > > > > mkfs.erofs, but this field leaves 0 for now. Is that reasonable? > > (using libuuid can generate it easily...) > > If the filed is always zero for now please don't fill it out. If you > decide it is worth adding the uuid eventually please add a compat > feature flag that you have a valid uuid and only fill out the field > if the file system actualy has a valid uuid. Okay. Will do that then (as a note here). Thanks, Gao Xiang ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v8 11/24] erofs: introduce xattr & posixacl support
Hi Christoph, On Mon, Sep 02, 2019 at 08:06:40AM -0700, Christoph Hellwig wrote: > On Mon, Sep 02, 2019 at 04:20:37PM +0200, 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. > > Given that the erofs folks have an actual use case for it I think > it is fine to keep the options, I just wanted to ensure this wasn't > copy and pasted for no good reason. Note that for XFS we don't have > an option for xattrs, as those are integral to a lot of file system > features. WE have an ACL option mostly for historic reasons. Add a word, xattr is necessary for Android, but ACL is not. I'm fine with killing these two, but EROFS can be used in more wider embedded scenerios (other than Android), I have no idea xattrs is useful for those embedded devices... Thanks, Gao Xiang ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v8 11/24] erofs: introduce xattr & posixacl support
Hi Christoph, On Mon, Sep 02, 2019 at 08:06:40AM -0700, Christoph Hellwig wrote: > On Mon, Sep 02, 2019 at 04:20:37PM +0200, 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. > > Given that the erofs folks have an actual use case for it I think > it is fine to keep the options, I just wanted to ensure this wasn't > copy and pasted for no good reason. Note that for XFS we don't have > an option for xattrs, as those are integral to a lot of file system > features. WE have an ACL option mostly for historic reasons. I fully understand your starting point... Thanks, Gao Xiang ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v6 03/24] erofs: add super block operations
Hi Christoph, On Mon, Sep 02, 2019 at 05:51:09AM -0700, Christoph Hellwig wrote: > On Sun, Sep 01, 2019 at 04:54:55PM +0800, Gao Xiang wrote: > > No modification at this... (some comments already right here...) > > > 20 /* 128-byte erofs on-disk super block */ > > 21 struct erofs_super_block { > > ... > > 24 __le32 features;/* (aka. feature_compat) */ > > ... > > 38 __le32 requirements;/* (aka. feature_incompat) */ > > ... > > 41 }; > > This is only cosmetic, why not stick to feature_compat and > feature_incompat? Okay, will fix. (however, in my mind, I'm some confused why "features" could be incompatible...) > > > > > + bh = sb_bread(sb, 0); > > > > > > Is there any good reasons to use buffer heads like this in new code > > > vs directly using bios? > > > > As you said, I want it in the page cache. > > > > The reason "why not use read_mapping_page or similar?" is simply > > read_mapping_page -> .readpage -> (for bdev inode) block_read_full_page > > -> create_page_buffers anyway... > > > > sb_bread haven't obsoleted... It has similar function though... > > With the different that it keeps you isolated from the buffer_head > internals. This seems to be your only direct use of buffer heads, > which while not deprecated are a bit of an ugly step child. So if > you can easily avoid creating a buffer_head dependency in a new > filesystem I think you should avoid it. OK, let's use read_mapping_page instead. > > > > > + sbi->build_time = le64_to_cpu(layout->build_time); > > > > + sbi->build_time_nsec = le32_to_cpu(layout->build_time_nsec); > > > > + > > > > + memcpy(>s_uuid, layout->uuid, sizeof(layout->uuid)); > > > > + memcpy(sbi->volume_name, layout->volume_name, > > > > + sizeof(layout->volume_name)); > > > > > > s_uuid should preferably be a uuid_t (assuming it is a real BE uuid, > > > if it is le it should be a guid_t). > > > > For this case, I have no idea how to deal with... > > I have little knowledge about this uuid stuff, so I just copied > > from f2fs... (Could be no urgent of this field...) > > Who fills out this field in the on-disk format and how? mkfs.erofs, but this field leaves 0 for now. Is that reasonable? (using libuuid can generate it easily...) > > > The background is Al's comments in erofs v2 > > (which simplify erofs_fill_super logic) > > https://lore.kernel.org/r/20190720224955.gd17...@zeniv.linux.org.uk/ > > > > with a specific notation... > > https://lore.kernel.org/r/20190721040547.gf17...@zeniv.linux.org.uk/ > > > > " > > > OTOH, for the case of NULL ->s_root ->put_super() won't be called > > > at all, so in that case you need it directly in ->kill_sb(). > > " > > Yes. Although none of that is relevant for this initial version, > just after more features are added. This patch uses it actually... since no failure path in erofs_fill_super() and s_root will be filled nearly at the end of the function... Thanks, Gao Xiang ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 00/21] erofs: patchset addressing Christoph's comments
Hi Christoph, On Mon, Sep 02, 2019 at 05:46:45AM -0700, Christoph Hellwig wrote: > On Sun, Sep 01, 2019 at 01:51:09PM +0800, 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. > > I like where the cleanups are going. But this is really just basic For now, I think it will go to Greg's tree. Before that, I will do patchset v2 to address all remaining comments... > code quality stuff. We're not addressing the issues with large amounts > of functionality duplicating VFS helpers. You means killing erofs_get_meta_page or avoid erofs_read_raw_page? - For killing erofs_get_meta_page, here is the current erofs_get_meta_page: 35 struct page *erofs_get_meta_page(struct super_block *sb, erofs_blk_t blkaddr) 36 { 37 struct inode *const bd_inode = sb->s_bdev->bd_inode; 38 struct address_space *const mapping = bd_inode->i_mapping; 39 /* prefer retrying in the allocator to blindly looping below */ 40 const gfp_t gfp = mapping_gfp_constraint(mapping, ~__GFP_FS); 41 struct page *page; 42 int err; 43 44 repeat: 45 page = find_or_create_page(mapping, blkaddr, gfp); 46 if (!page) 47 return ERR_PTR(-ENOMEM); 48 49 DBG_BUGON(!PageLocked(page)); 50 51 if (!PageUptodate(page)) { 52 struct bio *bio; 53 54 bio = erofs_grab_bio(sb, REQ_OP_READ | REQ_META, 55 blkaddr, 1, sb, erofs_readendio); 56 57 if (bio_add_page(bio, page, PAGE_SIZE, 0) != PAGE_SIZE) { 58 err = -EFAULT; 59 goto err_out; 60 } 61 62 submit_bio(bio); 63 lock_page(page); 64 65 /* this page has been truncated by others */ 66 if (page->mapping != mapping) { 67 unlock_page(page); 68 put_page(page); 69 goto repeat; 70 } 71 72 /* more likely a read error */ 73 if (!PageUptodate(page)) { 74 err = -EIO; 75 goto err_out; 76 } 77 } 78 return page; 79 80 err_out: 81 unlock_page(page); 82 put_page(page); 83 return ERR_PTR(err); 84 } I think it is simple enough. read_cache_page need write a similar filler, or read_cache_page_gfp will call .readpage, and then introduce buffer_heads, that is what I'd like to avoid now (no need these bd_inode buffer_heads in memory...) - For erofs_read_raw_page, it can be avoided after iomap tail-end packing feature is done... If we remove it now, it will make EROFS broken. It is no urgent and Chao will focus on iomap tail-end packing feature. Thanks, Gao Xiang ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v6 05/24] erofs: add inode operations
Hi David, On Mon, Sep 02, 2019 at 03:43:29PM +0200, David Sterba wrote: > On Sun, Sep 01, 2019 at 05:34:00PM +0800, Gao Xiang wrote: > > > > +static int read_inode(struct inode *inode, void *data) > > > > +{ > > > > + struct erofs_vnode *vi = EROFS_V(inode); > > > > + struct erofs_inode_v1 *v1 = data; > > > > + const unsigned int advise = le16_to_cpu(v1->i_advise); > > > > + erofs_blk_t nblks = 0; > > > > + > > > > + vi->datamode = __inode_data_mapping(advise); > > > > > > What is the deal with these magic underscores here and various > > > other similar helpers? > > > > Fixed in > > https://lore.kernel.org/linux-fsdevel/20190901055130.30572-17-hsiang...@aol.com/ > > > > underscores means 'internal' in my thought, it seems somewhat > > some common practice of Linux kernel, or some recent discussions > > about it?... I didn't notice these discussions... > > I know about a few valid uses of the underscores: > > * pattern where the __underscored version does not do locking, while the other > does > * similarly for atomic and non-atomic version > * macro that needs to manipulate the argument name (like glue some > prefix, so the macro does not have underscores and is supposed to be > used instead of the function with underscores that needs the full name > of a variable/constant/.. > * underscore function takes a few more parameters to further tune the > behaviour, but most users are fine with the defaults and that is > provided as a function without underscores > * in case you have just one function of the kind, don't use the underscores > > I can lookup examples if you're interested or if the brief description > is not sufficient. The list covers what I've seen and used, but the list > may be incomplete. Thanks, I learn a lot from the above. [thumb] Thanks, Gao Xiang ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 16/21] erofs: kill magic underscores
Hi Christoph, On Mon, Sep 02, 2019 at 05:54:38AM -0700, Christoph Hellwig wrote: > On Mon, Sep 02, 2019 at 08:39:37PM +0800, Gao Xiang wrote: > > > > > > > + if (erofs_inode_version(advise) == EROFS_INODE_LAYOUT_V2) { > > > > > > I still need to wade through the old thread - but didn't you say this > > > wasn't really a new vs old version but a compat vs full inode? Maybe > > > the names aren't that suitable either? > > > > Could you kindly give some suggestions for better naming about it? > > there are all supported by EROFS... (Actually we only mainly use v1...) > > Maybe EROFS_INODE_LAYOUT_COMPACT and EROFS_INODE_LAYOUT_EXTENDED? Okay, that is fine. Thanks, Gao Xiang ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 07/21] erofs: use erofs_inode naming
Hi Christoph, On Mon, Sep 02, 2019 at 05:47:37AM -0700, Christoph Hellwig wrote: > On Mon, Sep 02, 2019 at 08:13:06PM +0800, Gao Xiang wrote: > > Hi Christoph, > > > > On Mon, Sep 02, 2019 at 05:10:21AM -0700, Christoph Hellwig wrote: > > > > { > > > > - struct erofs_vnode *vi = ptr; > > > > - > > > > - inode_init_once(>vfs_inode); > > > > + inode_init_once(&((struct erofs_inode *)ptr)->vfs_inode); > > > > > > Why doesn't this use EROFS_I? This looks a little odd. > > > > Thanks for your reply and suggestion... > > EROFS_I seems the revert direction ---> inode to erofs_inode > > here we need "erofs_inode" to inode... > > > > Am I missing something? Hope not > > No, you are not. But the cast still looks odd. Why not: > > struct erofs_inode *ei = ptr; > > inode_init_once(>vfs_inode); That is the old way, I thought you don't like the extra variable... https://lore.kernel.org/linux-erofs/20190830154551.ga11...@infradead.org/ I am ok with either form, anyway, let me use the old way Thanks, Gao Xiang ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v8 11/24] erofs: introduce xattr & posixacl support
Hi Christoph, 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 > > + <http://acl.bestbits.at/> 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? ...Android (like smartphones) will not use it at least (just my personal thought, an option though...) Thanks, Gao Xiang ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v6 01/24] erofs: add on-disk layout
Hi Christoph, On Mon, Sep 02, 2019 at 05:45:21AM -0700, Christoph Hellwig wrote: > On Sun, Sep 01, 2019 at 03:54:11PM +0800, Gao Xiang wrote: > > It could be better has a name though, because 1) erofs.mkfs uses this > > definition explicitly, and we keep this on-disk definition erofs_fs.h > > file up with erofs-utils. > > > > 2) For kernel use, first we have, > >datamode < EROFS_INODE_LAYOUT_MAX; and > >!erofs_inode_is_data_compressed, so there are only two mode here, > > 1) EROFS_INODE_FLAT_INLINE, > > 2) EROFS_INODE_FLAT_PLAIN > >if its datamode isn't EROFS_INODE_FLAT_INLINE (tail-end block packing), > >it should be EROFS_INODE_FLAT_PLAIN. > > > >The detailed logic in erofs_read_inode and > >erofs_map_blocks_flatmode > > Ok. At least the explicit numbering makes this a little more obvious > now. What seems fairly odd is that there are only various places that > check for some inode layouts/formats but nothing that does a switch > over all of them. (Maybe not explicitly for this part) erofs_map_blocks_flatmode() ... 97 nblocks = DIV_ROUND_UP(inode->i_size, PAGE_SIZE); 98 lastblk = nblocks - is_inode_flat_inline(inode); ^ here ... Believe me EROFS_INODE_FLAT_PLAIN is used widely for EROFS images (if EROFS_INODE_FLAT_INLINE tail-end packing is not suitable and no compression) > > > > why are we adding a legacy field to a brand new file system? > > > > The difference is just EROFS_INODE_FLAT_COMPRESSION_LEGACY doesn't > > have z_erofs_map_header, so it only supports default (4k clustersize) > > fixed-sized output compression rather than per-file setting, nothing > > special at all... > > It still seems odd to add a legacy field to a brand new file system. Since 4.19 EROFS only supports EROFS_INODE_FLAT_COMPRESSION_LEGACY (per-filesystem setting), we'd like to introduce per-file setting and more configration for future requirements > > > > structures, as that keeps it clear in everyones mind what needs to > > > stay persistent and what can be chenged easily. > > > > All fields in this file are on-disk representation by design > > (no logic for in-memory presentation). > > Ok, make sense.Maybe add a note to the top of the file comment > that this is the on-disk format. > > One little oddity is that erofs_inode_is_data_compressed is here, while > is_inode_flat_inline is in internal.h. There are arguments for either > place, but I'd suggest to keep the related macros together. (Just my personal thought... erofs_inode_is_data_compressed operates ondisk field like datamode (because we have 2 datamode for compression, need to wrap them to judge if the file is compressed...) so it stays at erofs_fs.h... is_inode_flat_inline operates in-memory struct inode so it in internal.h) Thanks, Gao Xiang ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 20/21] erofs: kill use_vmap module parameter
Hi Christoph, On Mon, Sep 02, 2019 at 05:31:24AM -0700, Christoph Hellwig wrote: > > @@ -224,9 +220,6 @@ static void *erofs_vmap(struct page **pages, unsigned > > int count) > > { > > int i = 0; > > > > - if (use_vmap) > > - return vmap(pages, count, VM_MAP, PAGE_KERNEL); > > - > > while (1) { > > void *addr = vm_map_ram(pages, count, -1, PAGE_KERNEL); > > I think you can just open code this in the caller. Yes, the only one user... will fix... > > > static void erofs_vunmap(const void *mem, unsigned int count) > > { > > - if (!use_vmap) > > - vm_unmap_ram(mem, count); > > - else > > - vunmap(mem); > > + vm_unmap_ram(mem, count); > > } > > And this wrapper can go away entirely. Got it. will fix. > > And don't forget to report your performance observations to the arm64 > maintainers! In my observation, vm_map_ram always performs better... If there are something strange later, I will report to them immediately... :) Thanks, Gao Xiang ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 16/21] erofs: kill magic underscores
Hi Christoph, On Mon, Sep 02, 2019 at 05:26:27AM -0700, Christoph Hellwig wrote: > > > > - vi->datamode = __inode_data_mapping(advise); > > + vi->datamode = erofs_inode_data_mapping(advise); > > > > if (vi->datamode >= EROFS_INODE_LAYOUT_MAX) { > > While you are at it can we aim for some naming consistency here? The > inode member is called is called datamode, the helper is called > inode_data_mapping, and the enum uses layout? To me data_layout seems > most descriptive, datamode is probably ok, but mapping seems very > misleading given that we've already overloaded that terms for multiple > other uses. the naming changed for many times... Initially, it was called "data_mapping_mode", and I think it was too long (and as you said confusing, since confusing "mapping" meaning, sorry about my awkward English) so I simplified some into datamode In a word, I will change all data_mapping_mode into datalayout > > And while we are at naming choices - maybe i_format might be > a better name for the i_advise field in the on-disk inode? That is a good idea, I will resend v2 to address it... > > > + if (erofs_inode_version(advise) == EROFS_INODE_LAYOUT_V2) { > > I still need to wade through the old thread - but didn't you say this > wasn't really a new vs old version but a compat vs full inode? Maybe > the names aren't that suitable either? Could you kindly give some suggestions for better naming about it? there are all supported by EROFS... (Actually we only mainly use v1...) > > > struct erofs_inode_v2 *v2 = data; > > > > vi->inode_isize = sizeof(struct erofs_inode_v2); > > @@ -58,7 +58,7 @@ static int read_inode(struct inode *inode, void *data) > > /* total blocks for compressed files */ > > if (erofs_inode_is_data_compressed(vi->datamode)) > > nblks = le32_to_cpu(v2->i_u.compressed_blocks); > > - } else if (__inode_version(advise) == EROFS_INODE_LAYOUT_V1) { > > + } else if (erofs_inode_version(advise) == EROFS_INODE_LAYOUT_V1) { > > Also a lot of code would use a switch statements to switch for different > matches on the same value instead of chained if/else if/else if > statements. I will change it as well. > > > +#define erofs_bitrange(x, bit, bits) (((x) >> (bit)) & ((1 << (bits)) - 1)) > > > +#define erofs_inode_version(advise)\ > > + erofs_bitrange(advise, EROFS_I_VERSION_BIT, EROFS_I_VERSION_BITS) > > > > +#define erofs_inode_data_mapping(advise) \ > > + erofs_bitrange(advise, EROFS_I_DATA_MAPPING_BIT, \ > > + EROFS_I_DATA_MAPPING_BITS) > > All these should probably be inline functions. Will fix... Thanks, Gao Xiang ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 12/21] erofs: kill verbose debug info in erofs_fill_super
Hi Christoph, On Mon, Sep 02, 2019 at 05:14:24AM -0700, Christoph Hellwig wrote: > On Sun, Sep 01, 2019 at 01:51:21PM +0800, Gao Xiang wrote: > > From: Gao Xiang > > > > As Christoph said [1], "That is some very verbose > > debug info. We usually don't add that and let > > people trace the function instead. " > > Note that this applies to most of the infoln users as far as > I can tell. And if you want to keep some of those I think you > should converted them to use pr_info directly, and also print > sb->s_id as a prefix before the actual message so that the user > knows which file system is affected. Thanks for your suggestion... I think I will turn them into erofs_errln and etc... and print sb->s_id as a prefix... Thanks, Gao Xiang ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 11/21] erofs: use dsb instead of layout for ondisk super_block
Hi Christoph, On Mon, Sep 02, 2019 at 05:12:34AM -0700, Christoph Hellwig wrote: > > + dsb = (struct erofs_super_block *)((u8 *)bh->b_data + > > + EROFS_SUPER_OFFSET); > > Not new in this patch, but that u8 cast shouldn't be needed. Yes, thanks and will fix it... Thanks, Gao Xiang ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 07/21] erofs: use erofs_inode naming
Hi Christoph, On Mon, Sep 02, 2019 at 05:10:21AM -0700, Christoph Hellwig wrote: > > { > > - struct erofs_vnode *vi = ptr; > > - > > - inode_init_once(>vfs_inode); > > + inode_init_once(&((struct erofs_inode *)ptr)->vfs_inode); > > Why doesn't this use EROFS_I? This looks a little odd. Thanks for your reply and suggestion... EROFS_I seems the revert direction ---> inode to erofs_inode here we need "erofs_inode" to inode... Am I missing something? Hope not Thanks, Gao Xiang ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v6 01/24] erofs: add on-disk layout
Hi Pavel, (Thanks...) On Mon, Sep 02, 2019 at 10:40:20AM +0200, Pavel Machek wrote: > > So __packed is right thing to do. If architecture accesses that > slowly, that's ungood, but different structures between architectures > would be really bad. (...a little word, it seems that Christoph was trying to say that it's unnecessary to __packed for this case since we designed most erofs on-disk format in natural alignment... Anyway, I updated, that seems okay...) Thanks, Gao Xiang > > Best regards, > Pavel > -- > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v6 06/24] erofs: support special inode
Hi Christoph, On Thu, Aug 29, 2019 at 03:25:03AM -0700, Christoph Hellwig wrote: > On Fri, Aug 02, 2019 at 08:53:29PM +0800, Gao Xiang wrote: > > This patch adds to support special inode, such as > > block dev, char, socket, pipe inode. > > > > Signed-off-by: Gao Xiang > > --- > > fs/erofs/inode.c | 27 +-- > > 1 file changed, 25 insertions(+), 2 deletions(-) > > > > diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c > > index b6ea997bc4ae..637bf6e4de44 100644 > > --- a/fs/erofs/inode.c > > +++ b/fs/erofs/inode.c > > @@ -34,7 +34,16 @@ static int read_inode(struct inode *inode, void *data) > > vi->xattr_isize = ondisk_xattr_ibody_size(v2->i_xattr_icount); > > > > inode->i_mode = le16_to_cpu(v2->i_mode); > > - vi->raw_blkaddr = le32_to_cpu(v2->i_u.raw_blkaddr); > > + if (S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode) || > > + S_ISLNK(inode->i_mode)) > > + vi->raw_blkaddr = le32_to_cpu(v2->i_u.raw_blkaddr); > > + else if (S_ISCHR(inode->i_mode) || S_ISBLK(inode->i_mode)) > > + inode->i_rdev = > > + new_decode_dev(le32_to_cpu(v2->i_u.rdev)); > > + else if (S_ISFIFO(inode->i_mode) || S_ISSOCK(inode->i_mode)) > > + inode->i_rdev = 0; > > + else > > + return -EIO; > > Please use a switch statement when dealing with the file modes to > make everything easier to read. Fixed in https://lore.kernel.org/linux-fsdevel/20190901055130.30572-18-hsiang...@aol.com/ Thanks, Gao Xiang ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v6 05/24] erofs: add inode operations
Hi Christoph, Here are redo-ed comments of your suggestions... On Thu, Aug 29, 2019 at 03:24:26AM -0700, Christoph Hellwig wrote: > On Fri, Aug 02, 2019 at 08:53:28PM +0800, Gao Xiang wrote: > > This adds core functions to get, read an inode. > > It adds statx support as well. > > > > Signed-off-by: Gao Xiang > > --- > > fs/erofs/inode.c | 291 +++ > > 1 file changed, 291 insertions(+) > > create mode 100644 fs/erofs/inode.c > > > > diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c > > new file mode 100644 > > index ..b6ea997bc4ae > > --- /dev/null > > +++ b/fs/erofs/inode.c > > @@ -0,0 +1,291 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +/* > > + * linux/fs/erofs/inode.c > > + * > > + * Copyright (C) 2017-2018 HUAWEI, Inc. > > + * http://www.huawei.com/ > > + * Created by Gao Xiang > > + */ > > +#include "internal.h" > > + > > +#include > > + > > +/* no locking */ > > +static int read_inode(struct inode *inode, void *data) > > +{ > > + struct erofs_vnode *vi = EROFS_V(inode); > > + struct erofs_inode_v1 *v1 = data; > > + const unsigned int advise = le16_to_cpu(v1->i_advise); > > + erofs_blk_t nblks = 0; > > + > > + vi->datamode = __inode_data_mapping(advise); > > What is the deal with these magic underscores here and various > other similar helpers? Fixed in https://lore.kernel.org/linux-fsdevel/20190901055130.30572-17-hsiang...@aol.com/ underscores means 'internal' in my thought, it seems somewhat some common practice of Linux kernel, or some recent discussions about it?... I didn't notice these discussions... > > > + /* fast symlink (following ext4) */ > > This actually originates in FFS. But it is so common that the comment > seems a little pointless. Fixed in https://lore.kernel.org/linux-fsdevel/20190901055130.30572-9-hsiang...@aol.com/ > > > + if (S_ISLNK(inode->i_mode) && inode->i_size < PAGE_SIZE) { > > + char *lnk = erofs_kmalloc(sbi, inode->i_size + 1, GFP_KERNEL); > > Please just use plain kmalloc everywhere and let the normal kernel > error injection code take care of injeting any errors. Fixed in https://lore.kernel.org/linux-fsdevel/20190901055130.30572-20-hsiang...@aol.com/ > > > + /* inline symlink data shouldn't across page boundary as well */ > > ... should not cross .. Fixed in https://lore.kernel.org/linux-fsdevel/20190901055130.30572-9-hsiang...@aol.com/ > > > + if (unlikely(m_pofs + inode->i_size > PAGE_SIZE)) { > > + DBG_BUGON(1); > > + kfree(lnk); > > + return -EIO; > > + } > > + > > + /* get in-page inline data */ > > s/get/copy/, but the comment seems rather pointless. Fixed in https://lore.kernel.org/linux-fsdevel/20190901055130.30572-9-hsiang...@aol.com/ > > > + memcpy(lnk, data + m_pofs, inode->i_size); > > + lnk[inode->i_size] = '\0'; > > + > > + inode->i_link = lnk; > > + set_inode_fast_symlink(inode); > > Please just set the ops directly instead of obsfucating that in a single > caller, single line inline function. And please set it instead of the > normal symlink iops in the same place where you also set those.:w Fixed in https://lore.kernel.org/linux-fsdevel/20190901055130.30572-10-hsiang...@aol.com/ > > > + err = read_inode(inode, data + ofs); > > + if (!err) { > > if (err) > goto out_unlock; > > .. and save one level of indentation. Fixed in https://lore.kernel.org/linux-fsdevel/20190901055130.30572-22-hsiang...@aol.com/ > > > + if (is_inode_layout_compression(inode)) { > > The name of this helper is a little odd. But I think just > opencoding it seems generally cleaner anyway. Fixed in https://lore.kernel.org/linux-fsdevel/20190901055130.30572-11-hsiang...@aol.com/ > > > > + err = -ENOTSUPP; > > + goto out_unlock; > > + } > > + > > + inode->i_mapping->a_ops = _raw_access_aops; > > + > > + /* fill last page if inline data is available */ > > + err = fill_inline_data(inode, data, ofs); > > Well, I think you should move the is_inode_flat_inline and > (S_ISLNK(inode->i_mode) && inode->i_size < PAGE_SIZE) checks from that > helper here, as otherwise you make everyone wonder why you'd always > fill out the inline data.
Re: [PATCH v6 03/24] erofs: add super block operations
Hi Christoph, Here is also my redo-ed comments... On Thu, Aug 29, 2019 at 03:15:45AM -0700, Christoph Hellwig wrote: > On Fri, Aug 02, 2019 at 08:53:26PM +0800, Gao Xiang wrote: > > +static int __init erofs_init_inode_cache(void) > > +{ > > + erofs_inode_cachep = kmem_cache_create("erofs_inode", > > + sizeof(struct erofs_vnode), 0, > > + SLAB_RECLAIM_ACCOUNT, > > + init_once); > > + > > + return erofs_inode_cachep ? 0 : -ENOMEM; > > Please just use normal if/else. Also having this function seems > entirely pointless. Fixed in https://lore.kernel.org/r/20190901055130.30572-7-hsiang...@aol.com/ > > > +static void erofs_exit_inode_cache(void) > > +{ > > + kmem_cache_destroy(erofs_inode_cachep); > > +} > > Same for this one. Fixed in https://lore.kernel.org/r/20190901055130.30572-7-hsiang...@aol.com/ > > > +static void free_inode(struct inode *inode) > > Please use an erofs_ prefix for all your functions. free_inode and most short, common static functions are fixed in https://lore.kernel.org/r/20190901055130.30572-19-hsiang...@aol.com/ For all non-static functions, all are prefixed with "erofs_" > > > +{ > > + struct erofs_vnode *vi = EROFS_V(inode); > > Why is this called vnode instead of inode? That seems like a rather > odd naming for a Linux file system. Fixed in https://lore.kernel.org/r/20190901055130.30572-8-hsiang...@aol.com/ > > > + > > + /* be careful RCU symlink path (see ext4_inode_info->i_data)! */ > > + if (is_inode_fast_symlink(inode)) > > + kfree(inode->i_link); > > is_inode_fast_symlink only shows up in a later patch. And really > obsfucates the check here in the only caller as you can just do an > unconditional kfree here - i_link will be NULL except for the case > where you explicitly set it. Fixed in https://lore.kernel.org/r/20190901055130.30572-10-hsiang...@aol.com/ and with my following comments https://lore.kernel.org/r/20190831005446.GA233871@architecture4/ > > Also this code is nothing like ext4, so the code seems a little confusing. > > > +static bool check_layout_compatibility(struct super_block *sb, > > + struct erofs_super_block *layout) > > +{ > > + const unsigned int requirements = le32_to_cpu(layout->requirements); > > Why is the variable name for the on-disk subperblock layout? We usually > still calls this something with sb in the name, e.g. dsb. for disk > super block. Fixed in https://lore.kernel.org/r/20190901055130.30572-12-hsiang...@aol.com/ > > > + EROFS_SB(sb)->requirements = requirements; > > + > > + /* check if current kernel meets all mandatory requirements */ > > + if (requirements & (~EROFS_ALL_REQUIREMENTS)) { > > + errln("unidentified requirements %x, please upgrade kernel > > version", > > + requirements & ~EROFS_ALL_REQUIREMENTS); > > + return false; > > + } > > + return true; > > Note that normally we call this features, but that doesn't really > matter too much. No modification at this... (some comments already right here...) 20 /* 128-byte erofs on-disk super block */ 21 struct erofs_super_block { ... 24 __le32 features;/* (aka. feature_compat) */ ... 38 __le32 requirements;/* (aka. feature_incompat) */ ... 41 }; > > > +static int superblock_read(struct super_block *sb) > > +{ > > + struct erofs_sb_info *sbi; > > + struct buffer_head *bh; > > + struct erofs_super_block *layout; > > + unsigned int blkszbits; > > + int ret; > > + > > + bh = sb_bread(sb, 0); > > Is there any good reasons to use buffer heads like this in new code > vs directly using bios? As you said, I want it in the page cache. The reason "why not use read_mapping_page or similar?" is simply read_mapping_page -> .readpage -> (for bdev inode) block_read_full_page -> create_page_buffers anyway... sb_bread haven't obsoleted... It has similar function though... > > > + > > + sbi->blocks = le32_to_cpu(layout->blocks); > > + sbi->meta_blkaddr = le32_to_cpu(layout->meta_blkaddr); > > + sbi->islotbits = ffs(sizeof(struct erofs_inode_v1)) - 1; > > + sbi->root_nid = le16_to_cpu(layout->root_nid); > > + sbi->inos = le64_to_cpu(layout->inos); > > + > > + sbi->build_time = le64_to_cpu(layout->build_time); > > + sbi->build_time_nsec = le32_to_cpu(layout->bui
Re: [PATCH v6 01/24] erofs: add on-disk layout
ed in https://lore.kernel.org/linux-fsdevel/20190901055130.30572-5-hsiang...@aol.com/ > > > + > > +/* 32 bytes on-disk inode */ > > +#define EROFS_INODE_LAYOUT_V1 0 > > +/* 64 bytes on-disk inode */ > > +#define EROFS_INODE_LAYOUT_V2 1 > > + > > +struct erofs_inode_v2 { > > +/* 0 */__le16 i_advise; > > Why do we have two inode version in a newly added file system? There is no new or old, both can be used for the current EROFS in one image. v2 is an exhanced on-disk inode form, it has 64 bytes, v1 is more reduced one, which is already suitable for Android use case. > > > +#define ondisk_xattr_ibody_size(count) ({\ > > + u32 __count = le16_to_cpu(count); \ > > + ((__count) == 0) ? 0 : \ > > + sizeof(struct erofs_xattr_ibody_header) + \ > > + sizeof(__u32) * ((__count) - 1); }) > > This would be much more readable as a function. Fixed in https://lore.kernel.org/linux-fsdevel/20190901055130.30572-4-hsiang...@aol.com/ > > > +#define EROFS_XATTR_ENTRY_SIZE(entry) EROFS_XATTR_ALIGN( \ > > + sizeof(struct erofs_xattr_entry) + \ > > + (entry)->e_name_len + le16_to_cpu((entry)->e_value_size)) > > Same here. Fixed in https://lore.kernel.org/linux-fsdevel/20190901055130.30572-4-hsiang...@aol.com/ > > > +/* available compression algorithm types */ > > +enum { > > + Z_EROFS_COMPRESSION_LZ4, > > + Z_EROFS_COMPRESSION_MAX > > +}; > > Seems like an on-disk value again that should use explicitly assigned > numbers. Fixed in https://lore.kernel.org/linux-fsdevel/20190901055130.30572-3-hsiang...@aol.com/ Thanks, Gao Xiang ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 21/21] erofs: save one level of indentation
From: Gao Xiang As Christoph said [1], ".. and save one level of indentation." [1] https://lore.kernel.org/r/20190829102426.ge20...@infradead.org/ Reported-by: Christoph Hellwig Signed-off-by: Gao Xiang --- fs/erofs/inode.c | 65 1 file changed, 33 insertions(+), 32 deletions(-) diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c index fcc16d2d10cb..ee39b32bb911 100644 --- a/fs/erofs/inode.c +++ b/fs/erofs/inode.c @@ -188,41 +188,42 @@ static int erofs_fill_inode(struct inode *inode, int isdir) data = page_address(page); err = erofs_read_inode(inode, data + ofs); - if (!err) { - /* setup the new inode */ - switch (inode->i_mode & S_IFMT) { - case S_IFREG: - inode->i_op = _generic_iops; - inode->i_fop = _ro_fops; - break; - case S_IFDIR: - inode->i_op = _dir_iops; - inode->i_fop = _dir_fops; - break; - case S_IFLNK: - err = erofs_fill_symlink(inode, data, ofs); - if (err) - goto out_unlock; - inode_nohighmem(inode); - break; - case S_IFCHR: - case S_IFBLK: - case S_IFIFO: - case S_IFSOCK: - inode->i_op = _generic_iops; - init_special_inode(inode, inode->i_mode, inode->i_rdev); - goto out_unlock; - default: - err = -EFSCORRUPTED; + if (err) + goto out_unlock; + + /* setup the new inode */ + switch (inode->i_mode & S_IFMT) { + case S_IFREG: + inode->i_op = _generic_iops; + inode->i_fop = _ro_fops; + break; + case S_IFDIR: + inode->i_op = _dir_iops; + inode->i_fop = _dir_fops; + break; + case S_IFLNK: + err = erofs_fill_symlink(inode, data, ofs); + if (err) goto out_unlock; - } + inode_nohighmem(inode); + break; + case S_IFCHR: + case S_IFBLK: + case S_IFIFO: + case S_IFSOCK: + inode->i_op = _generic_iops; + init_special_inode(inode, inode->i_mode, inode->i_rdev); + goto out_unlock; + default: + err = -EFSCORRUPTED; + goto out_unlock; + } - if (erofs_inode_is_data_compressed(vi->datamode)) { - err = z_erofs_fill_inode(inode); - goto out_unlock; - } - inode->i_mapping->a_ops = _raw_access_aops; + if (erofs_inode_is_data_compressed(vi->datamode)) { + err = z_erofs_fill_inode(inode); + goto out_unlock; } + inode->i_mapping->a_ops = _raw_access_aops; out_unlock: unlock_page(page); -- 2.17.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 18/21] erofs: add "erofs_" prefix for common and short functions
From: Gao Xiang Add erofs_ prefix to free_inode, alloc_inode, ... Reported-by: Christoph Hellwig Signed-off-by: Gao Xiang --- fs/erofs/data.c | 6 +++--- fs/erofs/decompressor.c | 22 +++--- fs/erofs/inode.c| 8 fs/erofs/namei.c| 10 +- fs/erofs/super.c| 26 +- fs/erofs/zdata.c| 19 ++- 6 files changed, 46 insertions(+), 45 deletions(-) diff --git a/fs/erofs/data.c b/fs/erofs/data.c index d3cd7a453648..e8325eaa0fd8 100644 --- a/fs/erofs/data.c +++ b/fs/erofs/data.c @@ -9,7 +9,7 @@ #include -static inline void read_endio(struct bio *bio) +static void erofs_readendio(struct bio *bio) { struct super_block *const sb = bio->bi_private; struct bio_vec *bvec; @@ -58,7 +58,7 @@ struct page *erofs_get_meta_page(struct super_block *sb, erofs_blk_t blkaddr) struct bio *bio; bio = erofs_grab_bio(sb, REQ_OP_READ | REQ_META, -blkaddr, 1, sb, read_endio); +blkaddr, 1, sb, erofs_readendio); if (bio_add_page(bio, page, PAGE_SIZE, 0) != PAGE_SIZE) { err = -EFAULT; @@ -260,7 +260,7 @@ static inline struct bio *erofs_read_raw_page(struct bio *bio, nblocks = BIO_MAX_PAGES; bio = erofs_grab_bio(sb, REQ_OP_READ, blknr, nblocks, -sb, read_endio); +sb, erofs_readendio); } err = bio_add_page(bio, page, PAGE_SIZE, 0); diff --git a/fs/erofs/decompressor.c b/fs/erofs/decompressor.c index df349888f911..bb2944c96c89 100644 --- a/fs/erofs/decompressor.c +++ b/fs/erofs/decompressor.c @@ -32,8 +32,8 @@ static bool use_vmap; module_param(use_vmap, bool, 0444); MODULE_PARM_DESC(use_vmap, "Use vmap() instead of vm_map_ram() (default 0)"); -static int lz4_prepare_destpages(struct z_erofs_decompress_req *rq, -struct list_head *pagepool) +static int z_erofs_lz4_prepare_destpages(struct z_erofs_decompress_req *rq, +struct list_head *pagepool) { const unsigned int nr = PAGE_ALIGN(rq->pageofs_out + rq->outputsize) >> PAGE_SHIFT; @@ -114,7 +114,7 @@ static void *generic_copy_inplace_data(struct z_erofs_decompress_req *rq, return tmp; } -static int lz4_decompress(struct z_erofs_decompress_req *rq, u8 *out) +static int z_erofs_lz4_decompress(struct z_erofs_decompress_req *rq, u8 *out) { unsigned int inputmargin, inlen; u8 *src; @@ -187,8 +187,8 @@ static struct z_erofs_decompressor decompressors[] = { .name = "shifted" }, [Z_EROFS_COMPRESSION_LZ4] = { - .prepare_destpages = lz4_prepare_destpages, - .decompress = lz4_decompress, + .prepare_destpages = z_erofs_lz4_prepare_destpages, + .decompress = z_erofs_lz4_decompress, .name = "lz4" }, }; @@ -246,8 +246,8 @@ static void erofs_vunmap(const void *mem, unsigned int count) vunmap(mem); } -static int decompress_generic(struct z_erofs_decompress_req *rq, - struct list_head *pagepool) +static int z_erofs_decompress_generic(struct z_erofs_decompress_req *rq, + struct list_head *pagepool) { const unsigned int nrpages_out = PAGE_ALIGN(rq->pageofs_out + rq->outputsize) >> PAGE_SHIFT; @@ -307,8 +307,8 @@ static int decompress_generic(struct z_erofs_decompress_req *rq, return ret; } -static int shifted_decompress(const struct z_erofs_decompress_req *rq, - struct list_head *pagepool) +static int z_erofs_shifted_transform(const struct z_erofs_decompress_req *rq, +struct list_head *pagepool) { const unsigned int nrpages_out = PAGE_ALIGN(rq->pageofs_out + rq->outputsize) >> PAGE_SHIFT; @@ -352,7 +352,7 @@ int z_erofs_decompress(struct z_erofs_decompress_req *rq, struct list_head *pagepool) { if (rq->alg == Z_EROFS_COMPRESSION_SHIFTED) - return shifted_decompress(rq, pagepool); - return decompress_generic(rq, pagepool); + return z_erofs_shifted_transform(rq, pagepool); + return z_erofs_decompress_generic(rq, pagepool); } diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c index 6e2486cc3cd4..7da5a41f82e3 100644 --- a/fs/erofs/inode.c +++ b/fs/erofs/inode.c @@ -9,7 +9,7 @@ #include /* no locking */ -static int read_inode(struct inode *inode, void *data) +static int erofs_read_inode(struct inode *inode, void *data) { struct erofs_inode *vi = EROFS_I(inode); struct erofs_inode_v1 *v1 = dat
[PATCH 19/21] erofs: kill all erofs specific fault injection
From: Gao Xiang As Christoph suggested [1], "Please just use plain kmalloc everywhere and let the normal kernel error injection code take care of injeting any errors." [1] https://lore.kernel.org/r/20190829102426.ge20...@infradead.org/ Reported-by: Christoph Hellwig Signed-off-by: Gao Xiang --- Documentation/filesystems/erofs.txt | 5 --- fs/erofs/Kconfig| 7 --- fs/erofs/data.c | 6 --- fs/erofs/inode.c| 3 +- fs/erofs/internal.h | 65 --- fs/erofs/super.c| 70 - fs/erofs/zdata.c| 8 +--- 7 files changed, 2 insertions(+), 162 deletions(-) diff --git a/Documentation/filesystems/erofs.txt b/Documentation/filesystems/erofs.txt index 38aa9126ec98..c3b5f603b2b6 100644 --- a/Documentation/filesystems/erofs.txt +++ b/Documentation/filesystems/erofs.txt @@ -52,11 +52,6 @@ linux-erofs mailing list: Mount options = -fault_injection=%d Enable fault injection in all supported types with - specified injection rate. Supported injection type: - Type_NameType_Value - FAULT_KMALLOC0x1 - FAULT_READ_IO0x2 (no)user_xattr Setup Extended User Attributes. Note: xattr is enabled by default if CONFIG_EROFS_FS_XATTR is selected. (no)aclSetup POSIX Access Control List. Note: acl is enabled diff --git a/fs/erofs/Kconfig b/fs/erofs/Kconfig index 16316d1adca3..9d634d3a1845 100644 --- a/fs/erofs/Kconfig +++ b/fs/erofs/Kconfig @@ -27,13 +27,6 @@ config EROFS_FS_DEBUG For daily use, say N. -config EROFS_FAULT_INJECTION - bool "EROFS fault injection facility" - depends on EROFS_FS - help - Test EROFS to inject faults such as ENOMEM, EIO, and so on. - If unsure, say N. - config EROFS_FS_XATTR bool "EROFS extended attributes" depends on EROFS_FS diff --git a/fs/erofs/data.c b/fs/erofs/data.c index e8325eaa0fd8..a2a5ea945482 100644 --- a/fs/erofs/data.c +++ b/fs/erofs/data.c @@ -11,16 +11,10 @@ static void erofs_readendio(struct bio *bio) { - struct super_block *const sb = bio->bi_private; struct bio_vec *bvec; blk_status_t err = bio->bi_status; struct bvec_iter_all iter_all; - if (time_to_inject(EROFS_SB(sb), FAULT_READ_IO)) { - erofs_show_injection_info(FAULT_READ_IO); - err = BLK_STS_IOERR; - } - bio_for_each_segment_all(bvec, bio, iter_all) { struct page *page = bvec->bv_page; diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c index 7da5a41f82e3..fcc16d2d10cb 100644 --- a/fs/erofs/inode.c +++ b/fs/erofs/inode.c @@ -127,11 +127,10 @@ static int erofs_fill_symlink(struct inode *inode, void *data, unsigned int m_pofs) { struct erofs_inode *vi = EROFS_I(inode); - struct erofs_sb_info *sbi = EROFS_I_SB(inode); /* if it can be handled with fast symlink scheme */ if (is_inode_flat_inline(inode) && inode->i_size < PAGE_SIZE) { - char *lnk = erofs_kmalloc(sbi, inode->i_size + 1, GFP_KERNEL); + char *lnk = kmalloc(inode->i_size + 1, GFP_KERNEL); if (!lnk) return -ENOMEM; diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h index 4a35a31fd454..f5fb5ba52fe2 100644 --- a/fs/erofs/internal.h +++ b/fs/erofs/internal.h @@ -32,23 +32,6 @@ #define DBG_BUGON(x)((void)(x)) #endif /* !CONFIG_EROFS_FS_DEBUG */ -enum { - FAULT_KMALLOC, - FAULT_READ_IO, - FAULT_MAX, -}; - -#ifdef CONFIG_EROFS_FAULT_INJECTION -extern const char *erofs_fault_name[FAULT_MAX]; -#define IS_FAULT_SET(fi, type) ((fi)->inject_type & (1 << (type))) - -struct erofs_fault_info { - atomic_t inject_ops; - unsigned int inject_rate; - unsigned int inject_type; -}; -#endif /* CONFIG_EROFS_FAULT_INJECTION */ - /* EROFS_SUPER_MAGIC_V1 to represent the whole file system */ #define EROFS_SUPER_MAGIC EROFS_SUPER_MAGIC_V1 @@ -99,62 +82,14 @@ struct erofs_sb_info { u32 requirements; unsigned int mount_opt; - -#ifdef CONFIG_EROFS_FAULT_INJECTION - struct erofs_fault_info fault_info; /* For fault injection */ -#endif }; -#ifdef CONFIG_EROFS_FAULT_INJECTION -#define erofs_show_injection_info(type) \ - infoln("inject %s in %s of %pS", erofs_fault_name[type],\ - __func__, __builtin_return_address(0)) - -static inline bool time_to_inject(struct erofs_sb_info *sbi, int type) -{ - struct erofs_fault_info *ffi = >fault_info; - - if (!ffi->inject_rate) -
[PATCH 15/21] erofs: kill __submit_bio()
From: Gao Xiang As Christoph pointed out [1], " Why is there __submit_bio which really just obsfucates what is going on? Also why is __submit_bio using bio_set_op_attrs instead of opencode it as the comment right next to it asks you to? " Let's use submit_bio directly instead. [1] https://lore.kernel.org/lkml/20190830162812.ga10...@infradead.org/ Reported-by: Christoph Hellwig Signed-off-by: Gao Xiang --- fs/erofs/data.c | 15 --- fs/erofs/internal.h | 9 ++--- fs/erofs/zdata.c| 6 +++--- 3 files changed, 13 insertions(+), 17 deletions(-) diff --git a/fs/erofs/data.c b/fs/erofs/data.c index 837b07cd2761..d3cd7a453648 100644 --- a/fs/erofs/data.c +++ b/fs/erofs/data.c @@ -57,14 +57,15 @@ struct page *erofs_get_meta_page(struct super_block *sb, erofs_blk_t blkaddr) if (!PageUptodate(page)) { struct bio *bio; - bio = erofs_grab_bio(sb, blkaddr, 1, sb, read_endio); + bio = erofs_grab_bio(sb, REQ_OP_READ | REQ_META, +blkaddr, 1, sb, read_endio); if (bio_add_page(bio, page, PAGE_SIZE, 0) != PAGE_SIZE) { err = -EFAULT; goto err_out; } - __submit_bio(bio, REQ_OP_READ, REQ_META); + submit_bio(bio); lock_page(page); /* this page has been truncated by others */ @@ -188,7 +189,7 @@ static inline struct bio *erofs_read_raw_page(struct bio *bio, /* not continuous */ *last_block + 1 != current_block) { submit_bio_retry: - __submit_bio(bio, REQ_OP_READ, 0); + submit_bio(bio); bio = NULL; } @@ -258,7 +259,8 @@ static inline struct bio *erofs_read_raw_page(struct bio *bio, if (nblocks > BIO_MAX_PAGES) nblocks = BIO_MAX_PAGES; - bio = erofs_grab_bio(sb, blknr, nblocks, sb, read_endio); + bio = erofs_grab_bio(sb, REQ_OP_READ, blknr, nblocks, +sb, read_endio); } err = bio_add_page(bio, page, PAGE_SIZE, 0); @@ -289,8 +291,7 @@ static inline struct bio *erofs_read_raw_page(struct bio *bio, /* if updated manually, continuous pages has a gap */ if (bio) submit_bio_out: - __submit_bio(bio, REQ_OP_READ, 0); - + submit_bio(bio); return err ? ERR_PTR(err) : NULL; } @@ -354,7 +355,7 @@ static int erofs_raw_access_readpages(struct file *filp, /* the rare case (end in gaps) */ if (bio) - __submit_bio(bio, REQ_OP_READ, 0); + submit_bio(bio); return 0; } diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h index 40d4dd47bb7a..15545959af92 100644 --- a/fs/erofs/internal.h +++ b/fs/erofs/internal.h @@ -406,6 +406,7 @@ static inline int z_erofs_map_blocks_iter(struct inode *inode, /* data.c */ static inline struct bio *erofs_grab_bio(struct super_block *sb, +unsigned int bi_opf, erofs_blk_t blkaddr, unsigned int nr_pages, void *bi_private, bio_end_io_t endio) @@ -416,16 +417,10 @@ static inline struct bio *erofs_grab_bio(struct super_block *sb, bio_set_dev(bio, sb->s_bdev); bio->bi_iter.bi_sector = (sector_t)blkaddr << LOG_SECTORS_PER_BLOCK; bio->bi_private = bi_private; + bio->bi_opf = bi_opf; return bio; } -static inline void __submit_bio(struct bio *bio, unsigned int op, - unsigned int op_flags) -{ - bio_set_op_attrs(bio, op, op_flags); - submit_bio(bio); -} - struct page *erofs_get_meta_page(struct super_block *sb, erofs_blk_t blkaddr); int erofs_map_blocks(struct inode *, struct erofs_map_blocks *, int); diff --git a/fs/erofs/zdata.c b/fs/erofs/zdata.c index abe28565d6c0..ce1a0f2997a9 100644 --- a/fs/erofs/zdata.c +++ b/fs/erofs/zdata.c @@ -1258,12 +1258,12 @@ static bool z_erofs_vle_submit_all(struct super_block *sb, if (bio && force_submit) { submit_bio_retry: - __submit_bio(bio, REQ_OP_READ, 0); + submit_bio(bio); bio = NULL; } if (!bio) { - bio = erofs_grab_bio(sb, first_index + i, + bio = erofs_grab_bio(sb, REQ_OP_READ, first_index + i, BIO_MAX_PAGES, bi_private, z_erofs_vle_read_endio); ++nr_bios; @@ -1286,7 +1286,7 @@ static bool z_erofs_vle_submit_all(struct super_block *sb, } while (owned_head != Z_EROFS_PCLUSTER_TAIL); if (bio) -
[PATCH 14/21] erofs: kill prio and nofail of erofs_get_meta_page()
From: Gao Xiang As Christoph pointed out [1], "Why is there __erofs_get_meta_page with the two weird booleans instead of a single erofs_get_meta_page that gets and gfp_t for additional flags and an unsigned int for additional bio op flags." And since all callers can handle errors, let's kill prio and nofail and erofs_get_inline_page() now. [1] https://lore.kernel.org/lkml/20190830162812.ga10...@infradead.org/ Reported-by: Christoph Hellwig Signed-off-by: Gao Xiang --- fs/erofs/data.c | 24 ++-- fs/erofs/inode.c| 2 +- fs/erofs/internal.h | 18 +- fs/erofs/xattr.c| 13 ++--- fs/erofs/zmap.c | 4 ++-- 5 files changed, 16 insertions(+), 45 deletions(-) diff --git a/fs/erofs/data.c b/fs/erofs/data.c index 621954d4fb09..837b07cd2761 100644 --- a/fs/erofs/data.c +++ b/fs/erofs/data.c @@ -38,25 +38,20 @@ static inline void read_endio(struct bio *bio) bio_put(bio); } -/* prio -- true is used for dir */ -struct page *__erofs_get_meta_page(struct super_block *sb, - erofs_blk_t blkaddr, bool prio, bool nofail) +struct page *erofs_get_meta_page(struct super_block *sb, erofs_blk_t blkaddr) { struct inode *const bd_inode = sb->s_bdev->bd_inode; struct address_space *const mapping = bd_inode->i_mapping; /* prefer retrying in the allocator to blindly looping below */ - const gfp_t gfp = mapping_gfp_constraint(mapping, ~__GFP_FS) | - (nofail ? __GFP_NOFAIL : 0); - unsigned int io_retries = nofail ? EROFS_IO_MAX_RETRIES_NOFAIL : 0; + const gfp_t gfp = mapping_gfp_constraint(mapping, ~__GFP_FS); struct page *page; int err; repeat: page = find_or_create_page(mapping, blkaddr, gfp); - if (!page) { - DBG_BUGON(nofail); + if (!page) return ERR_PTR(-ENOMEM); - } + DBG_BUGON(!PageLocked(page)); if (!PageUptodate(page)) { @@ -69,14 +64,11 @@ struct page *__erofs_get_meta_page(struct super_block *sb, goto err_out; } - __submit_bio(bio, REQ_OP_READ, -REQ_META | (prio ? REQ_PRIO : 0)); - + __submit_bio(bio, REQ_OP_READ, REQ_META); lock_page(page); /* this page has been truncated by others */ if (page->mapping != mapping) { -unlock_repeat: unlock_page(page); put_page(page); goto repeat; @@ -84,10 +76,6 @@ struct page *__erofs_get_meta_page(struct super_block *sb, /* more likely a read error */ if (!PageUptodate(page)) { - if (io_retries) { - --io_retries; - goto unlock_repeat; - } err = -EIO; goto err_out; } @@ -237,7 +225,7 @@ static inline struct bio *erofs_read_raw_page(struct bio *bio, DBG_BUGON(map.m_plen > PAGE_SIZE); - ipage = erofs_get_meta_page(inode->i_sb, blknr, 0); + ipage = erofs_get_meta_page(inode->i_sb, blknr); if (IS_ERR(ipage)) { err = PTR_ERR(ipage); diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c index d501ceb62c29..19a574ee690b 100644 --- a/fs/erofs/inode.c +++ b/fs/erofs/inode.c @@ -163,7 +163,7 @@ static int fill_inode(struct inode *inode, int isdir) debugln("%s, reading inode nid %llu at %u of blkaddr %u", __func__, vi->nid, ofs, blkaddr); - page = erofs_get_meta_page(inode->i_sb, blkaddr, isdir); + page = erofs_get_meta_page(inode->i_sb, blkaddr); if (IS_ERR(page)) { errln("failed to get inode (nid: %llu) page, err %ld", diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h index 01749be24f3d..40d4dd47bb7a 100644 --- a/fs/erofs/internal.h +++ b/fs/erofs/internal.h @@ -258,8 +258,6 @@ static inline int erofs_wait_on_workgroup_freezed(struct erofs_workgroup *grp) #error erofs cannot be used in this platform #endif -#define EROFS_IO_MAX_RETRIES_NOFAIL 5 - #define ROOT_NID(sb) ((sb)->root_nid) #define erofs_blknr(addr) ((addr) / EROFS_BLKSIZ) @@ -428,24 +426,10 @@ static inline void __submit_bio(struct bio *bio, unsigned int op, submit_bio(bio); } -struct page *__erofs_get_meta_page(struct super_block *sb, erofs_blk_t blkaddr, - bool prio, bool nofail); - -static inline struct page *erofs_get_meta_page(struct super_block *sb, - erofs_blk_t blkaddr, bool prio) -{ - return __erofs_get_meta_page(sb, blkaddr, prio, false); -} +struct page *erofs_get_meta_page(struct super_block *sb, erofs_blk_t blkaddr);
[PATCH 20/21] erofs: kill use_vmap module parameter
From: Gao Xiang As Christoph said [1], "vm_map_ram is supposed to generally behave better. So if it doesn't please report that that to the arch maintainer and linux-mm so that they can look into the issue. Having user make choices of deep down kernel internals is just a horrible interface. Please talk to maintainers of other bits of the kernel if you see issues and / or need enhancements. " Let's redo the previous conclusion and kill the vmap approach. [1] https://lore.kernel.org/r/20190830165533.ga10...@infradead.org/ Reported-by: Christoph Hellwig Signed-off-by: Gao Xiang --- Documentation/filesystems/erofs.txt | 4 fs/erofs/decompressor.c | 12 +--- 2 files changed, 1 insertion(+), 15 deletions(-) diff --git a/Documentation/filesystems/erofs.txt b/Documentation/filesystems/erofs.txt index c3b5f603b2b6..b0c085326e2e 100644 --- a/Documentation/filesystems/erofs.txt +++ b/Documentation/filesystems/erofs.txt @@ -67,10 +67,6 @@ cache_strategy=%s Select a strategy for cached decompression from now on: It still does in-place I/O decompression for the rest compressed physical clusters. -Module parameters -= -use_vmap=[0|1] Use vmap() instead of vm_map_ram() (default 0). - On-disk details === diff --git a/fs/erofs/decompressor.c b/fs/erofs/decompressor.c index bb2944c96c89..af273d89e62c 100644 --- a/fs/erofs/decompressor.c +++ b/fs/erofs/decompressor.c @@ -28,10 +28,6 @@ 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)"); - static int z_erofs_lz4_prepare_destpages(struct z_erofs_decompress_req *rq, struct list_head *pagepool) { @@ -224,9 +220,6 @@ static void *erofs_vmap(struct page **pages, unsigned int count) { int i = 0; - if (use_vmap) - return vmap(pages, count, VM_MAP, PAGE_KERNEL); - while (1) { void *addr = vm_map_ram(pages, count, -1, PAGE_KERNEL); @@ -240,10 +233,7 @@ static void *erofs_vmap(struct page **pages, unsigned int count) static void erofs_vunmap(const void *mem, unsigned int count) { - if (!use_vmap) - vm_unmap_ram(mem, count); - else - vunmap(mem); + vm_unmap_ram(mem, count); } static int z_erofs_decompress_generic(struct z_erofs_decompress_req *rq, -- 2.17.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 17/21] erofs: use a switch statement when dealing with the file modes
From: Gao Xiang As Christoph suggested [1], " Please use a switch statement when dealing with the file modes to make everything easier to read." [1] https://lore.kernel.org/r/20190829102503.gf20...@infradead.org/ Reported-by: Christoph Hellwig Signed-off-by: Gao Xiang --- fs/erofs/inode.c | 38 ++ 1 file changed, 26 insertions(+), 12 deletions(-) diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c index 2ca4eda6e5bf..6e2486cc3cd4 100644 --- a/fs/erofs/inode.c +++ b/fs/erofs/inode.c @@ -32,17 +32,24 @@ static int read_inode(struct inode *inode, void *data) vi->xattr_isize = erofs_xattr_ibody_size(v2->i_xattr_icount); inode->i_mode = le16_to_cpu(v2->i_mode); - if (S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode) || - S_ISLNK(inode->i_mode)) + switch (inode->i_mode & S_IFMT) { + case S_IFREG: + case S_IFDIR: + case S_IFLNK: vi->raw_blkaddr = le32_to_cpu(v2->i_u.raw_blkaddr); - else if (S_ISCHR(inode->i_mode) || S_ISBLK(inode->i_mode)) + break; + case S_IFCHR: + case S_IFBLK: inode->i_rdev = new_decode_dev(le32_to_cpu(v2->i_u.rdev)); - else if (S_ISFIFO(inode->i_mode) || S_ISSOCK(inode->i_mode)) + break; + case S_IFIFO: + case S_IFSOCK: inode->i_rdev = 0; - else + break; + default: goto bogusimode; - + } i_uid_write(inode, le32_to_cpu(v2->i_uid)); i_gid_write(inode, le32_to_cpu(v2->i_gid)); set_nlink(inode, le32_to_cpu(v2->i_nlink)); @@ -65,17 +72,24 @@ static int read_inode(struct inode *inode, void *data) vi->xattr_isize = erofs_xattr_ibody_size(v1->i_xattr_icount); inode->i_mode = le16_to_cpu(v1->i_mode); - if (S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode) || - S_ISLNK(inode->i_mode)) + switch (inode->i_mode & S_IFMT) { + case S_IFREG: + case S_IFDIR: + case S_IFLNK: vi->raw_blkaddr = le32_to_cpu(v1->i_u.raw_blkaddr); - else if (S_ISCHR(inode->i_mode) || S_ISBLK(inode->i_mode)) + break; + case S_IFCHR: + case S_IFBLK: inode->i_rdev = new_decode_dev(le32_to_cpu(v1->i_u.rdev)); - else if (S_ISFIFO(inode->i_mode) || S_ISSOCK(inode->i_mode)) + break; + case S_IFIFO: + case S_IFSOCK: inode->i_rdev = 0; - else + break; + default: goto bogusimode; - + } i_uid_write(inode, le16_to_cpu(v1->i_uid)); i_gid_write(inode, le16_to_cpu(v1->i_gid)); set_nlink(inode, le16_to_cpu(v1->i_nlink)); -- 2.17.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 16/21] erofs: kill magic underscores
From: Gao Xiang As Christoph said [1], " > + vi->datamode = __inode_data_mapping(advise); What is the deal with these magic underscores here and various other similar helpers? " Let avoid magic underscores now... [1] https://lore.kernel.org/lkml/20190829102426.ge20...@infradead.org/ Reported-by: Christoph Hellwig Signed-off-by: Gao Xiang --- fs/erofs/inode.c| 8 fs/erofs/internal.h | 14 ++ 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c index 19a574ee690b..2ca4eda6e5bf 100644 --- a/fs/erofs/inode.c +++ b/fs/erofs/inode.c @@ -16,7 +16,7 @@ static int read_inode(struct inode *inode, void *data) const unsigned int advise = le16_to_cpu(v1->i_advise); erofs_blk_t nblks = 0; - vi->datamode = __inode_data_mapping(advise); + vi->datamode = erofs_inode_data_mapping(advise); if (vi->datamode >= EROFS_INODE_LAYOUT_MAX) { errln("unsupported data mapping %u of nid %llu", @@ -25,7 +25,7 @@ static int read_inode(struct inode *inode, void *data) return -EOPNOTSUPP; } - if (__inode_version(advise) == EROFS_INODE_LAYOUT_V2) { + if (erofs_inode_version(advise) == EROFS_INODE_LAYOUT_V2) { struct erofs_inode_v2 *v2 = data; vi->inode_isize = sizeof(struct erofs_inode_v2); @@ -58,7 +58,7 @@ static int read_inode(struct inode *inode, void *data) /* total blocks for compressed files */ if (erofs_inode_is_data_compressed(vi->datamode)) nblks = le32_to_cpu(v2->i_u.compressed_blocks); - } else if (__inode_version(advise) == EROFS_INODE_LAYOUT_V1) { + } else if (erofs_inode_version(advise) == EROFS_INODE_LAYOUT_V1) { struct erofs_sb_info *sbi = EROFS_SB(inode->i_sb); vi->inode_isize = sizeof(struct erofs_inode_v1); @@ -91,7 +91,7 @@ static int read_inode(struct inode *inode, void *data) nblks = le32_to_cpu(v1->i_u.compressed_blocks); } else { errln("unsupported on-disk inode version %u of nid %llu", - __inode_version(advise), vi->nid); + erofs_inode_version(advise), vi->nid); DBG_BUGON(1); return -EOPNOTSUPP; } diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h index 15545959af92..4a35a31fd454 100644 --- a/fs/erofs/internal.h +++ b/fs/erofs/internal.h @@ -308,16 +308,14 @@ struct erofs_inode { #define EROFS_I(ptr) \ container_of(ptr, struct erofs_inode, vfs_inode) -#define __inode_advise(x, bit, bits) \ - (((x) >> (bit)) & ((1 << (bits)) - 1)) +#define erofs_bitrange(x, bit, bits) (((x) >> (bit)) & ((1 << (bits)) - 1)) -#define __inode_version(advise)\ - __inode_advise(advise, EROFS_I_VERSION_BIT, \ - EROFS_I_VERSION_BITS) +#define erofs_inode_version(advise)\ + erofs_bitrange(advise, EROFS_I_VERSION_BIT, EROFS_I_VERSION_BITS) -#define __inode_data_mapping(advise) \ - __inode_advise(advise, EROFS_I_DATA_MAPPING_BIT,\ - EROFS_I_DATA_MAPPING_BITS) +#define erofs_inode_data_mapping(advise) \ + erofs_bitrange(advise, EROFS_I_DATA_MAPPING_BIT, \ + EROFS_I_DATA_MAPPING_BITS) static inline unsigned long inode_datablocks(struct inode *inode) { -- 2.17.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 13/21] erofs: simplify erofs_grab_bio() since bio_alloc() never fail
From: Gao Xiang As Christoph pointed out [1], "erofs_grab_bio tries to handle a bio_alloc failure, except that the function will not actually fail due the mempool backing it." Sorry about useless code, fix it now. [1] https://lore.kernel.org/lkml/20190830162812.ga10...@infradead.org/ Reported-by: Christoph Hellwig Signed-off-by: Gao Xiang --- fs/erofs/data.c | 15 ++- fs/erofs/internal.h | 19 ++- fs/erofs/zdata.c| 2 +- 3 files changed, 5 insertions(+), 31 deletions(-) diff --git a/fs/erofs/data.c b/fs/erofs/data.c index e2e40ec2bfd1..621954d4fb09 100644 --- a/fs/erofs/data.c +++ b/fs/erofs/data.c @@ -62,12 +62,7 @@ struct page *__erofs_get_meta_page(struct super_block *sb, if (!PageUptodate(page)) { struct bio *bio; - bio = erofs_grab_bio(sb, blkaddr, 1, sb, read_endio, nofail); - if (IS_ERR(bio)) { - DBG_BUGON(nofail); - err = PTR_ERR(bio); - goto err_out; - } + bio = erofs_grab_bio(sb, blkaddr, 1, sb, read_endio); if (bio_add_page(bio, page, PAGE_SIZE, 0) != PAGE_SIZE) { err = -EFAULT; @@ -275,13 +270,7 @@ static inline struct bio *erofs_read_raw_page(struct bio *bio, if (nblocks > BIO_MAX_PAGES) nblocks = BIO_MAX_PAGES; - bio = erofs_grab_bio(sb, blknr, nblocks, sb, -read_endio, false); - if (IS_ERR(bio)) { - err = PTR_ERR(bio); - bio = NULL; - goto err_out; - } + bio = erofs_grab_bio(sb, blknr, nblocks, sb, read_endio); } err = bio_add_page(bio, page, PAGE_SIZE, 0); diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h index 6bd82a82b11f..01749be24f3d 100644 --- a/fs/erofs/internal.h +++ b/fs/erofs/internal.h @@ -410,24 +410,9 @@ static inline int z_erofs_map_blocks_iter(struct inode *inode, static inline struct bio *erofs_grab_bio(struct super_block *sb, erofs_blk_t blkaddr, unsigned int nr_pages, -void *bi_private, bio_end_io_t endio, -bool nofail) +void *bi_private, bio_end_io_t endio) { - const gfp_t gfp = GFP_NOIO; - struct bio *bio; - - do { - if (nr_pages == 1) { - bio = bio_alloc(gfp | (nofail ? __GFP_NOFAIL : 0), 1); - if (!bio) { - DBG_BUGON(nofail); - return ERR_PTR(-ENOMEM); - } - break; - } - bio = bio_alloc(gfp, nr_pages); - nr_pages /= 2; - } while (!bio); + struct bio *bio = bio_alloc(GFP_NOIO, nr_pages); bio->bi_end_io = endio; bio_set_dev(bio, sb->s_bdev); diff --git a/fs/erofs/zdata.c b/fs/erofs/zdata.c index f06a2fad7af2..abe28565d6c0 100644 --- a/fs/erofs/zdata.c +++ b/fs/erofs/zdata.c @@ -1265,7 +1265,7 @@ static bool z_erofs_vle_submit_all(struct super_block *sb, if (!bio) { bio = erofs_grab_bio(sb, first_index + i, BIO_MAX_PAGES, bi_private, -z_erofs_vle_read_endio, true); +z_erofs_vle_read_endio); ++nr_bios; } -- 2.17.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 05/21] erofs: update erofs_inode_is_data_compressed helper
From: Gao Xiang As Christoph said, "This looks like a really obsfucated way to write: return datamode == EROFS_INODE_FLAT_COMPRESSION || datamode == EROFS_INODE_FLAT_COMPRESSION_LEGACY; " Although I had my own consideration, it's the right way for now. [1] https://lore.kernel.org/linux-fsdevel/20190829095954.gb20...@infradead.org/ Reported-by: Christoph Hellwig Signed-off-by: Gao Xiang --- fs/erofs/erofs_fs.h | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/fs/erofs/erofs_fs.h b/fs/erofs/erofs_fs.h index 59dcc2e8cb02..87d7ae82339a 100644 --- a/fs/erofs/erofs_fs.h +++ b/fs/erofs/erofs_fs.h @@ -62,9 +62,8 @@ enum { static inline bool erofs_inode_is_data_compressed(unsigned int datamode) { - if (datamode == EROFS_INODE_FLAT_COMPRESSION) - return true; - return datamode == EROFS_INODE_FLAT_COMPRESSION_LEGACY; + return datamode == EROFS_INODE_FLAT_COMPRESSION || + datamode == EROFS_INODE_FLAT_COMPRESSION_LEGACY; } /* bit definitions of inode i_advise */ -- 2.17.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 06/21] erofs: kill erofs_{init,exit}_inode_cache
From: Gao Xiang As Christoph said [1] "having this function seems entirely pointless", let's kill those. filesystem function name ext2,f2fs,ext4,isofs,squashfs,cifs,... init_inodecache In addition, add a necessary "rcu_barrier()" on exit_fs(); [1] https://lore.kernel.org/r/20190829101545.gc20...@infradead.org/ Reported-by: Christoph Hellwig Signed-off-by: Gao Xiang --- fs/erofs/super.c | 31 --- 1 file changed, 12 insertions(+), 19 deletions(-) diff --git a/fs/erofs/super.c b/fs/erofs/super.c index 6603f0ba8905..0c412de33315 100644 --- a/fs/erofs/super.c +++ b/fs/erofs/super.c @@ -23,21 +23,6 @@ static void init_once(void *ptr) inode_init_once(>vfs_inode); } -static int __init erofs_init_inode_cache(void) -{ - erofs_inode_cachep = kmem_cache_create("erofs_inode", - sizeof(struct erofs_vnode), 0, - SLAB_RECLAIM_ACCOUNT, - init_once); - - return erofs_inode_cachep ? 0 : -ENOMEM; -} - -static void erofs_exit_inode_cache(void) -{ - kmem_cache_destroy(erofs_inode_cachep); -} - static struct inode *alloc_inode(struct super_block *sb) { struct erofs_vnode *vi = @@ -531,9 +516,14 @@ static int __init erofs_module_init(void) erofs_check_ondisk_layout_definitions(); infoln("initializing erofs " EROFS_VERSION); - err = erofs_init_inode_cache(); - if (err) + erofs_inode_cachep = kmem_cache_create("erofs_inode", + sizeof(struct erofs_vnode), 0, + SLAB_RECLAIM_ACCOUNT, + init_once); + if (!erofs_inode_cachep) { + err = -ENOMEM; goto icache_err; + } err = erofs_init_shrinker(); if (err) @@ -555,7 +545,7 @@ static int __init erofs_module_init(void) zip_err: erofs_exit_shrinker(); shrinker_err: - erofs_exit_inode_cache(); + kmem_cache_destroy(erofs_inode_cachep); icache_err: return err; } @@ -565,7 +555,10 @@ static void __exit erofs_module_exit(void) unregister_filesystem(_fs_type); z_erofs_exit_zip_subsystem(); erofs_exit_shrinker(); - erofs_exit_inode_cache(); + + /* Ensure all RCU free inodes are safe before cache is destroyed. */ + rcu_barrier(); + kmem_cache_destroy(erofs_inode_cachep); infoln("successfully finalize erofs"); } -- 2.17.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 08/21] erofs: update comments in inode.c
From: Gao Xiang As Christoph suggested [1], update them all. [1] https://lore.kernel.org/r/20190829102426.ge20...@infradead.org/ Reported-by: Christoph Hellwig Signed-off-by: Gao Xiang --- fs/erofs/inode.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c index de0373647959..24c94a7865f2 100644 --- a/fs/erofs/inode.c +++ b/fs/erofs/inode.c @@ -129,7 +129,7 @@ static int fill_inline_data(struct inode *inode, void *data, if (!is_inode_flat_inline(inode)) return 0; - /* fast symlink (following ext4) */ + /* fast symlink */ if (S_ISLNK(inode->i_mode) && inode->i_size < PAGE_SIZE) { char *lnk = erofs_kmalloc(sbi, inode->i_size + 1, GFP_KERNEL); @@ -138,7 +138,7 @@ static int fill_inline_data(struct inode *inode, void *data, m_pofs += vi->inode_isize + vi->xattr_isize; - /* inline symlink data shouldn't across page boundary as well */ + /* inline symlink data shouldn't cross page boundary as well */ if (m_pofs + inode->i_size > PAGE_SIZE) { kfree(lnk); errln("inline data cross block boundary @ nid %llu", @@ -147,7 +147,6 @@ static int fill_inline_data(struct inode *inode, void *data, return -EFSCORRUPTED; } - /* get in-page inline data */ memcpy(lnk, data + m_pofs, inode->i_size); lnk[inode->i_size] = '\0'; -- 2.17.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 12/21] erofs: kill verbose debug info in erofs_fill_super
From: Gao Xiang As Christoph said [1], "That is some very verbose debug info. We usually don't add that and let people trace the function instead. " [1] https://lore.kernel.org/r/20190829101545.gc20...@infradead.org/ Reported-by: Christoph Hellwig Signed-off-by: Gao Xiang --- fs/erofs/super.c | 9 ++--- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/fs/erofs/super.c b/fs/erofs/super.c index c1a42ea7b72f..b4bf72755300 100644 --- a/fs/erofs/super.c +++ b/fs/erofs/super.c @@ -383,9 +383,6 @@ static int erofs_fill_super(struct super_block *sb, void *data, int silent) struct erofs_sb_info *sbi; int err; - infoln("fill_super, device -> %s", sb->s_id); - infoln("options -> %s", (char *)data); - sb->s_magic = EROFS_SUPER_MAGIC; if (!sb_set_blocksize(sb, EROFS_BLKSIZ)) { @@ -418,9 +415,6 @@ static int erofs_fill_super(struct super_block *sb, void *data, int silent) if (err) return err; - if (!silent) - infoln("root inode @ nid %llu", ROOT_NID(sbi)); - if (test_opt(sbi, POSIX_ACL)) sb->s_flags |= SB_POSIXACL; else @@ -453,7 +447,8 @@ static int erofs_fill_super(struct super_block *sb, void *data, int silent) return err; if (!silent) - infoln("mounted on %s with opts: %s.", sb->s_id, (char *)data); + infoln("mounted on %s with opts: %s, root inode @ nid %llu.", + sb->s_id, (char *)data, ROOT_NID(sbi)); return 0; } -- 2.17.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 04/21] erofs: kill __packed for on-disk structures
From: Gao Xiang As Christoph suggested "Please don't add __packed" [1], 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/r/20190829095954.gb20...@infradead.org/ Reported-by: Christoph Hellwig Signed-off-by: Gao Xiang --- fs/erofs/erofs_fs.h | 18 +- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/fs/erofs/erofs_fs.h b/fs/erofs/erofs_fs.h index c1220b0f26e0..59dcc2e8cb02 100644 --- a/fs/erofs/erofs_fs.h +++ b/fs/erofs/erofs_fs.h @@ -38,7 +38,7 @@ struct erofs_super_block { __le32 requirements;/* (aka. feature_incompat) */ __u8 reserved2[44]; -} __packed; +}; /* * erofs inode data mapping: @@ -91,12 +91,12 @@ struct erofs_inode_v1 { /* for device files, used to indicate old/new device # */ __le32 rdev; - } i_u __packed; + } i_u; __le32 i_ino; /* only used for 32-bit stat compatibility */ __le16 i_uid; __le16 i_gid; __le32 i_reserved2; -} __packed; +}; /* 32 bytes on-disk inode */ #define EROFS_INODE_LAYOUT_V1 0 @@ -119,7 +119,7 @@ struct erofs_inode_v2 { /* for device files, used to indicate old/new device # */ __le32 rdev; - } i_u __packed; + } i_u; /* only used for 32-bit stat compatibility */ __le32 i_ino; @@ -130,7 +130,7 @@ struct erofs_inode_v2 { __le32 i_ctime_nsec; __le32 i_nlink; __u8 i_reserved2[16]; -} __packed; +}; #define EROFS_MAX_SHARED_XATTRS (128) /* h_shared_count between 129 ... 255 are special # */ @@ -152,7 +152,7 @@ struct erofs_xattr_ibody_header { __u8 h_shared_count; __u8 h_reserved2[7]; __le32 h_shared_xattrs[0]; /* shared xattr id array */ -} __packed; +}; /* Name indexes */ #define EROFS_XATTR_INDEX_USER 1 @@ -169,7 +169,7 @@ struct erofs_xattr_entry { __le16 e_value_size;/* size of attribute value */ /* followed by e_name and e_value */ char e_name[0]; /* attribute name */ -} __packed; +}; static inline unsigned int erofs_xattr_ibody_size(__le16 i_xattr_icount) { @@ -273,8 +273,8 @@ struct z_erofs_vle_decompressed_index { * [1] - pointing to the tail cluster */ __le16 delta[2]; - } di_u __packed; -} __packed; + } di_u; +}; #define Z_EROFS_VLE_LEGACY_INDEX_ALIGN(size) \ (round_up(size, sizeof(struct z_erofs_vle_decompressed_index)) + \ -- 2.17.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 09/21] erofs: update erofs symlink stuffs
From: Gao Xiang Fix as Christoph suggested [1] [2], "remove is_inode_fast_symlink and just opencode it in the few places using it" and "Please just set the ops directly instead of obsfucating that in a single caller, single line inline function. And please set it instead of the normal symlink iops in the same place where you also set those." [1] https://lore.kernel.org/r/20190830163910.gb29...@infradead.org/ [2] https://lore.kernel.org/r/20190829102426.ge20...@infradead.org/ Reported-by: Christoph Hellwig Signed-off-by: Gao Xiang --- fs/erofs/inode.c| 35 ++- fs/erofs/internal.h | 10 -- fs/erofs/super.c| 5 ++--- 3 files changed, 12 insertions(+), 38 deletions(-) diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c index 24c94a7865f2..29a52138fa9d 100644 --- a/fs/erofs/inode.c +++ b/fs/erofs/inode.c @@ -109,28 +109,14 @@ static int read_inode(struct inode *inode, void *data) return -EFSCORRUPTED; } -/* - * try_lock can be required since locking order is: - * file data(fs_inode) - *meta(bd_inode) - * but the majority of the callers is "iget", - * in that case we are pretty sure no deadlock since - * no data operations exist. However I tend to - * try_lock since it takes no much overhead and - * will success immediately. - */ -static int fill_inline_data(struct inode *inode, void *data, - unsigned int m_pofs) +static int erofs_fill_symlink(struct inode *inode, void *data, + unsigned int m_pofs) { struct erofs_inode *vi = EROFS_I(inode); struct erofs_sb_info *sbi = EROFS_I_SB(inode); - /* should be inode inline C */ - if (!is_inode_flat_inline(inode)) - return 0; - - /* fast symlink */ - if (S_ISLNK(inode->i_mode) && inode->i_size < PAGE_SIZE) { + /* if it can be handled with fast symlink scheme */ + if (is_inode_flat_inline(inode) && inode->i_size < PAGE_SIZE) { char *lnk = erofs_kmalloc(sbi, inode->i_size + 1, GFP_KERNEL); if (!lnk) @@ -151,7 +137,9 @@ static int fill_inline_data(struct inode *inode, void *data, lnk[inode->i_size] = '\0'; inode->i_link = lnk; - set_inode_fast_symlink(inode); + inode->i_op = _fast_symlink_iops; + } else { + inode->i_op = _symlink_iops; } return 0; } @@ -199,8 +187,9 @@ static int fill_inode(struct inode *inode, int isdir) inode->i_fop = _dir_fops; break; case S_IFLNK: - /* by default, page_get_link is used for symlink */ - inode->i_op = _symlink_iops; + err = erofs_fill_symlink(inode, data, ofs); + if (err) + goto out_unlock; inode_nohighmem(inode); break; case S_IFCHR: @@ -219,11 +208,7 @@ static int fill_inode(struct inode *inode, int isdir) err = z_erofs_fill_inode(inode); goto out_unlock; } - inode->i_mapping->a_ops = _raw_access_aops; - - /* fill last page if inline data is available */ - err = fill_inline_data(inode, data, ofs); } out_unlock: diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h index e576650bd4f4..4442a6622504 100644 --- a/fs/erofs/internal.h +++ b/fs/erofs/internal.h @@ -480,16 +480,6 @@ extern const struct inode_operations erofs_generic_iops; extern const struct inode_operations erofs_symlink_iops; extern const struct inode_operations erofs_fast_symlink_iops; -static inline void set_inode_fast_symlink(struct inode *inode) -{ - inode->i_op = _fast_symlink_iops; -} - -static inline bool is_inode_fast_symlink(struct inode *inode) -{ - return inode->i_op == _fast_symlink_iops; -} - struct inode *erofs_iget(struct super_block *sb, erofs_nid_t nid, bool dir); int erofs_getattr(const struct path *path, struct kstat *stat, u32 request_mask, unsigned int query_flags); diff --git a/fs/erofs/super.c b/fs/erofs/super.c index b0d318a8eb22..8c43af5d5e57 100644 --- a/fs/erofs/super.c +++ b/fs/erofs/super.c @@ -38,10 +38,9 @@ static void free_inode(struct inode *inode) { struct erofs_inode *vi = EROFS_I(inode); - /* be careful RCU symlink path (see ext4_inode_info->i_data)! */ - if (is_inode_fast_symlink(inode)) + /* be careful of RCU symlink path */ + if (inode->i_op == _fast_symlink_iops) kfree(inode->i_link); - kfree(vi->xattr_shared_xattrs); kmem_cache_free(erofs_inode_cachep, vi); -- 2.17.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 07/21] erofs: use erofs_inode naming
From: Gao Xiang As Christoph suggested [1], "Why is this called vnode instead of inode? That seems like a rather odd naming for a Linux file system." [1] https://lore.kernel.org/r/20190829101545.gc20...@infradead.org/ Reported-by: Christoph Hellwig Signed-off-by: Gao Xiang --- fs/erofs/data.c | 4 ++-- fs/erofs/dir.c | 6 +++--- fs/erofs/inode.c | 10 +- fs/erofs/internal.h | 18 +- fs/erofs/namei.c | 2 +- fs/erofs/super.c | 12 +--- fs/erofs/xattr.c | 18 +- fs/erofs/xattr.h | 4 ++-- fs/erofs/zdata.c | 9 +++-- fs/erofs/zmap.c | 28 ++-- include/trace/events/erofs.h | 14 +++--- 11 files changed, 60 insertions(+), 65 deletions(-) diff --git a/fs/erofs/data.c b/fs/erofs/data.c index 0983807737fd..d736d2e551a1 100644 --- a/fs/erofs/data.c +++ b/fs/erofs/data.c @@ -112,7 +112,7 @@ static int erofs_map_blocks_flatmode(struct inode *inode, int err = 0; erofs_blk_t nblocks, lastblk; u64 offset = map->m_la; - struct erofs_vnode *vi = EROFS_V(inode); + struct erofs_inode *vi = EROFS_I(inode); trace_erofs_map_blocks_flatmode_enter(inode, map, flags); @@ -364,7 +364,7 @@ static int erofs_raw_access_readpages(struct file *filp, if (IS_ERR(bio)) { pr_err("%s, readahead error at page %lu of nid %llu\n", __func__, page->index, - EROFS_V(mapping->host)->nid); + EROFS_I(mapping->host)->nid); bio = NULL; } diff --git a/fs/erofs/dir.c b/fs/erofs/dir.c index 6a5b43f7fb29..a032c8217071 100644 --- a/fs/erofs/dir.c +++ b/fs/erofs/dir.c @@ -47,7 +47,7 @@ static int erofs_fill_dentries(struct inode *dir, struct dir_context *ctx, /* a corrupted entry is found */ if (nameoff + de_namelen > maxsize || de_namelen > EROFS_NAME_LEN) { - errln("bogus dirent @ nid %llu", EROFS_V(dir)->nid); + errln("bogus dirent @ nid %llu", EROFS_I(dir)->nid); DBG_BUGON(1); return -EFSCORRUPTED; } @@ -85,7 +85,7 @@ static int erofs_readdir(struct file *f, struct dir_context *ctx) break; } else if (IS_ERR(dentry_page)) { errln("fail to readdir of logical block %u of nid %llu", - i, EROFS_V(dir)->nid); + i, EROFS_I(dir)->nid); err = -EFSCORRUPTED; break; } @@ -97,7 +97,7 @@ static int erofs_readdir(struct file *f, struct dir_context *ctx) if (nameoff < sizeof(struct erofs_dirent) || nameoff >= PAGE_SIZE) { errln("%s, invalid de[0].nameoff %u @ nid %llu", - __func__, nameoff, EROFS_V(dir)->nid); + __func__, nameoff, EROFS_I(dir)->nid); err = -EFSCORRUPTED; goto skip_this; } diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c index 3fc4f764b387..de0373647959 100644 --- a/fs/erofs/inode.c +++ b/fs/erofs/inode.c @@ -11,7 +11,7 @@ /* no locking */ static int read_inode(struct inode *inode, void *data) { - struct erofs_vnode *vi = EROFS_V(inode); + struct erofs_inode *vi = EROFS_I(inode); struct erofs_inode_v1 *v1 = data; const unsigned int advise = le16_to_cpu(v1->i_advise); erofs_blk_t nblks = 0; @@ -122,7 +122,7 @@ static int read_inode(struct inode *inode, void *data) static int fill_inline_data(struct inode *inode, void *data, unsigned int m_pofs) { - struct erofs_vnode *vi = EROFS_V(inode); + struct erofs_inode *vi = EROFS_I(inode); struct erofs_sb_info *sbi = EROFS_I_SB(inode); /* should be inode inline C */ @@ -160,7 +160,7 @@ static int fill_inline_data(struct inode *inode, void *data, static int fill_inode(struct inode *inode, int isdir) { struct erofs_sb_info *sbi = EROFS_SB(inode->i_sb); - struct erofs_vnode *vi = EROFS_V(inode); + struct erofs_inode *vi = EROFS_I(inode); struct page *page; void *data; int err; @@ -242,7 +242,7 @@ static int erofs_ilookup_test_actor(struct inode *inode, void *opaque) { const erofs_nid_t nid = *(erofs_nid_t *)opaque; - return EROFS_V(inode)->nid == nid; + return EROFS_I(inode)->nid == nid; } static int erofs_iget_set_actor(str
[PATCH 03/21] erofs: some macros are much more readable as a function
From: Gao Xiang As Christoph suggested [1], these macros 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 --- fs/erofs/erofs_fs.h | 22 ++ fs/erofs/inode.c| 4 ++-- fs/erofs/xattr.c| 2 +- 3 files changed, 17 insertions(+), 11 deletions(-) diff --git a/fs/erofs/erofs_fs.h b/fs/erofs/erofs_fs.h index d1f152a3670a..c1220b0f26e0 100644 --- a/fs/erofs/erofs_fs.h +++ b/fs/erofs/erofs_fs.h @@ -171,16 +171,22 @@ struct erofs_xattr_entry { char e_name[0]; /* attribute name */ } __packed; -#define ondisk_xattr_ibody_size(count) ({\ - u32 __count = le16_to_cpu(count); \ - ((__count) == 0) ? 0 : \ - sizeof(struct erofs_xattr_ibody_header) + \ - sizeof(__u32) * ((__count) - 1); }) +static inline unsigned int erofs_xattr_ibody_size(__le16 i_xattr_icount) +{ + if (!i_xattr_icount) + return 0; + + return sizeof(struct erofs_xattr_ibody_header) + + sizeof(__u32) * (le16_to_cpu(i_xattr_icount) - 1); +} #define EROFS_XATTR_ALIGN(size) round_up(size, sizeof(struct erofs_xattr_entry)) -#define EROFS_XATTR_ENTRY_SIZE(entry) EROFS_XATTR_ALIGN( \ - sizeof(struct erofs_xattr_entry) + \ - (entry)->e_name_len + le16_to_cpu((entry)->e_value_size)) + +static inline unsigned int erofs_xattr_entry_size(struct erofs_xattr_entry *e) +{ + return EROFS_XATTR_ALIGN(sizeof(struct erofs_xattr_entry) + +e->e_name_len + le16_to_cpu(e->e_value_size)); +} /* available compression algorithm types */ enum { diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c index 8a0574530a0a..3fc4f764b387 100644 --- a/fs/erofs/inode.c +++ b/fs/erofs/inode.c @@ -29,7 +29,7 @@ static int read_inode(struct inode *inode, void *data) struct erofs_inode_v2 *v2 = data; vi->inode_isize = sizeof(struct erofs_inode_v2); - vi->xattr_isize = ondisk_xattr_ibody_size(v2->i_xattr_icount); + vi->xattr_isize = erofs_xattr_ibody_size(v2->i_xattr_icount); inode->i_mode = le16_to_cpu(v2->i_mode); if (S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode) || @@ -62,7 +62,7 @@ static int read_inode(struct inode *inode, void *data) struct erofs_sb_info *sbi = EROFS_SB(inode->i_sb); vi->inode_isize = sizeof(struct erofs_inode_v1); - vi->xattr_isize = ondisk_xattr_ibody_size(v1->i_xattr_icount); + vi->xattr_isize = erofs_xattr_ibody_size(v1->i_xattr_icount); inode->i_mode = le16_to_cpu(v1->i_mode); if (S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode) || diff --git a/fs/erofs/xattr.c b/fs/erofs/xattr.c index d80f61dde72f..620cbc15f4d0 100644 --- a/fs/erofs/xattr.c +++ b/fs/erofs/xattr.c @@ -231,7 +231,7 @@ static int xattr_foreach(struct xattr_iter *it, */ entry = *(struct erofs_xattr_entry *)(it->kaddr + it->ofs); if (tlimit) { - unsigned int entry_sz = EROFS_XATTR_ENTRY_SIZE(); + unsigned int entry_sz = erofs_xattr_entry_size(); /* xattr on-disk corruption: xattr entry beyond xattr_isize */ if (*tlimit < entry_sz) { -- 2.17.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 10/21] erofs: kill is_inode_layout_compression()
From: Gao Xiang As Christoph suggested [1], "The name of this helper is a little odd. But I think just opencoding it seems generally cleaner anyway. " [1] https://lore.kernel.org/r/20190829102426.ge20...@infradead.org/ Reported-by: Christoph Hellwig Signed-off-by: Gao Xiang --- fs/erofs/data.c | 2 +- fs/erofs/inode.c| 8 fs/erofs/internal.h | 5 - 3 files changed, 5 insertions(+), 10 deletions(-) diff --git a/fs/erofs/data.c b/fs/erofs/data.c index d736d2e551a1..e2e40ec2bfd1 100644 --- a/fs/erofs/data.c +++ b/fs/erofs/data.c @@ -169,7 +169,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 (is_inode_layout_compression(inode)) { + if (erofs_inode_is_data_compressed(EROFS_I(inode)->datamode)) { int err = z_erofs_map_blocks_iter(inode, map, flags); if (map->mpage) { diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c index 29a52138fa9d..d501ceb62c29 100644 --- a/fs/erofs/inode.c +++ b/fs/erofs/inode.c @@ -56,7 +56,7 @@ static int read_inode(struct inode *inode, void *data) inode->i_size = le64_to_cpu(v2->i_size); /* total blocks for compressed files */ - if (is_inode_layout_compression(inode)) + if (erofs_inode_is_data_compressed(vi->datamode)) nblks = le32_to_cpu(v2->i_u.compressed_blocks); } else if (__inode_version(advise) == EROFS_INODE_LAYOUT_V1) { struct erofs_sb_info *sbi = EROFS_SB(inode->i_sb); @@ -87,7 +87,7 @@ static int read_inode(struct inode *inode, void *data) sbi->build_time_nsec; inode->i_size = le32_to_cpu(v1->i_size); - if (is_inode_layout_compression(inode)) + if (erofs_inode_is_data_compressed(vi->datamode)) nblks = le32_to_cpu(v1->i_u.compressed_blocks); } else { errln("unsupported on-disk inode version %u of nid %llu", @@ -204,7 +204,7 @@ static int fill_inode(struct inode *inode, int isdir) goto out_unlock; } - if (is_inode_layout_compression(inode)) { + if (erofs_inode_is_data_compressed(vi->datamode)) { err = z_erofs_fill_inode(inode); goto out_unlock; } @@ -283,7 +283,7 @@ int erofs_getattr(const struct path *path, struct kstat *stat, { struct inode *const inode = d_inode(path->dentry); - if (is_inode_layout_compression(inode)) + if (erofs_inode_is_data_compressed(EROFS_I(inode)->datamode)) stat->attributes |= STATX_ATTR_COMPRESSED; stat->attributes |= STATX_ATTR_IMMUTABLE; diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h index 4442a6622504..6bd82a82b11f 100644 --- a/fs/erofs/internal.h +++ b/fs/erofs/internal.h @@ -327,11 +327,6 @@ static inline unsigned long inode_datablocks(struct inode *inode) return DIV_ROUND_UP(inode->i_size, EROFS_BLKSIZ); } -static inline bool is_inode_layout_compression(struct inode *inode) -{ - return erofs_inode_is_data_compressed(EROFS_I(inode)->datamode); -} - static inline bool is_inode_flat_inline(struct inode *inode) { return EROFS_I(inode)->datamode == EROFS_INODE_FLAT_INLINE; -- 2.17.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 11/21] erofs: use dsb instead of layout for ondisk super_block
From: Gao Xiang As Christoph pointed out [1], "Why is the variable name for the on-disk subperblock layout? We usually still calls this something with sb in the name, e.g. dsb. for disksuper block. " Let's fix it. [1] https://lore.kernel.org/r/20190829101545.gc20...@infradead.org/ Signed-off-by: Gao Xiang --- fs/erofs/super.c | 36 ++-- 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/fs/erofs/super.c b/fs/erofs/super.c index 8c43af5d5e57..c1a42ea7b72f 100644 --- a/fs/erofs/super.c +++ b/fs/erofs/super.c @@ -47,9 +47,9 @@ static void free_inode(struct inode *inode) } static bool check_layout_compatibility(struct super_block *sb, - struct erofs_super_block *layout) + struct erofs_super_block *dsb) { - const unsigned int requirements = le32_to_cpu(layout->requirements); + const unsigned int requirements = le32_to_cpu(dsb->requirements); EROFS_SB(sb)->requirements = requirements; @@ -66,7 +66,7 @@ static int superblock_read(struct super_block *sb) { struct erofs_sb_info *sbi; struct buffer_head *bh; - struct erofs_super_block *layout; + struct erofs_super_block *dsb; unsigned int blkszbits; int ret; @@ -78,16 +78,16 @@ static int superblock_read(struct super_block *sb) } sbi = EROFS_SB(sb); - layout = (struct erofs_super_block *)((u8 *)bh->b_data -+ EROFS_SUPER_OFFSET); + dsb = (struct erofs_super_block *)((u8 *)bh->b_data + + EROFS_SUPER_OFFSET); ret = -EINVAL; - if (le32_to_cpu(layout->magic) != EROFS_SUPER_MAGIC_V1) { + if (le32_to_cpu(dsb->magic) != EROFS_SUPER_MAGIC_V1) { errln("cannot find valid erofs superblock"); goto out; } - blkszbits = layout->blkszbits; + blkszbits = dsb->blkszbits; /* 9(512 bytes) + LOG_SECTORS_PER_BLOCK == LOG_BLOCK_SIZE */ if (blkszbits != LOG_BLOCK_SIZE) { errln("blksize %u isn't supported on this platform", @@ -95,25 +95,25 @@ static int superblock_read(struct super_block *sb) goto out; } - if (!check_layout_compatibility(sb, layout)) + if (!check_layout_compatibility(sb, dsb)) goto out; - sbi->blocks = le32_to_cpu(layout->blocks); - sbi->meta_blkaddr = le32_to_cpu(layout->meta_blkaddr); + sbi->blocks = le32_to_cpu(dsb->blocks); + sbi->meta_blkaddr = le32_to_cpu(dsb->meta_blkaddr); #ifdef CONFIG_EROFS_FS_XATTR - sbi->xattr_blkaddr = le32_to_cpu(layout->xattr_blkaddr); + sbi->xattr_blkaddr = le32_to_cpu(dsb->xattr_blkaddr); #endif sbi->islotbits = ffs(sizeof(struct erofs_inode_v1)) - 1; - sbi->root_nid = le16_to_cpu(layout->root_nid); - sbi->inos = le64_to_cpu(layout->inos); + sbi->root_nid = le16_to_cpu(dsb->root_nid); + sbi->inos = le64_to_cpu(dsb->inos); - sbi->build_time = le64_to_cpu(layout->build_time); - sbi->build_time_nsec = le32_to_cpu(layout->build_time_nsec); + sbi->build_time = le64_to_cpu(dsb->build_time); + sbi->build_time_nsec = le32_to_cpu(dsb->build_time_nsec); - memcpy(>s_uuid, layout->uuid, sizeof(layout->uuid)); + memcpy(>s_uuid, dsb->uuid, sizeof(dsb->uuid)); - ret = strscpy(sbi->volume_name, layout->volume_name, - sizeof(layout->volume_name)); + ret = strscpy(sbi->volume_name, dsb->volume_name, + sizeof(dsb->volume_name)); if (ret < 0) { /* -E2BIG */ errln("bad volume name without NIL terminator"); ret = -EFSCORRUPTED; -- 2.17.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 02/21] erofs: on-disk format should have explicitly assigned numbers
From: Gao Xiang 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 --- fs/erofs/erofs_fs.h | 18 +- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/fs/erofs/erofs_fs.h b/fs/erofs/erofs_fs.h index 49335fff9d65..d1f152a3670a 100644 --- a/fs/erofs/erofs_fs.h +++ b/fs/erofs/erofs_fs.h @@ -53,10 +53,10 @@ struct erofs_super_block { * 4~7 - reserved */ enum { - EROFS_INODE_FLAT_PLAIN, - EROFS_INODE_FLAT_COMPRESSION_LEGACY, - EROFS_INODE_FLAT_INLINE, - EROFS_INODE_FLAT_COMPRESSION, + EROFS_INODE_FLAT_PLAIN = 0, + EROFS_INODE_FLAT_COMPRESSION_LEGACY = 1, + EROFS_INODE_FLAT_INLINE = 2, + EROFS_INODE_FLAT_COMPRESSION= 3, EROFS_INODE_LAYOUT_MAX }; @@ -184,7 +184,7 @@ struct erofs_xattr_entry { /* available compression algorithm types */ enum { - Z_EROFS_COMPRESSION_LZ4, + Z_EROFS_COMPRESSION_LZ4 = 0, Z_EROFS_COMPRESSION_MAX }; @@ -242,10 +242,10 @@ struct z_erofs_map_header { *(di_advise could be 0, 1 or 2) */ enum { - Z_EROFS_VLE_CLUSTER_TYPE_PLAIN, - Z_EROFS_VLE_CLUSTER_TYPE_HEAD, - Z_EROFS_VLE_CLUSTER_TYPE_NONHEAD, - Z_EROFS_VLE_CLUSTER_TYPE_RESERVED, + Z_EROFS_VLE_CLUSTER_TYPE_PLAIN = 0, + Z_EROFS_VLE_CLUSTER_TYPE_HEAD = 1, + Z_EROFS_VLE_CLUSTER_TYPE_NONHEAD= 2, + Z_EROFS_VLE_CLUSTER_TYPE_RESERVED = 3, Z_EROFS_VLE_CLUSTER_TYPE_MAX }; -- 2.17.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 01/21] erofs: remove all the byte offset comments
From: Gao Xiang As Christoph suggested [1], "Please remove all the byte offset comments. that is something that can easily be checked with gdb or pahole." [1] https://lore.kernel.org/r/20190829095954.gb20...@infradead.org/ Reported-by: Christoph Hellwig Signed-off-by: Gao Xiang --- fs/erofs/erofs_fs.h | 105 +++- 1 file changed, 54 insertions(+), 51 deletions(-) diff --git a/fs/erofs/erofs_fs.h b/fs/erofs/erofs_fs.h index afa7d45ca958..49335fff9d65 100644 --- a/fs/erofs/erofs_fs.h +++ b/fs/erofs/erofs_fs.h @@ -17,27 +17,28 @@ #define EROFS_REQUIREMENT_LZ4_0PADDING 0x0001 #define EROFS_ALL_REQUIREMENTS EROFS_REQUIREMENT_LZ4_0PADDING +/* 128-byte erofs on-disk super block */ struct erofs_super_block { -/* 0 */__le32 magic; /* in the little endian */ -/* 4 */__le32 checksum;/* crc32c(super_block) */ -/* 8 */__le32 features;/* (aka. feature_compat) */ -/* 12 */__u8 blkszbits; /* support block_size == PAGE_SIZE only */ -/* 13 */__u8 reserved; - -/* 14 */__le16 root_nid; -/* 16 */__le64 inos;/* total valid ino # (== f_files - f_favail) */ - -/* 24 */__le64 build_time; /* inode v1 time derivation */ -/* 32 */__le32 build_time_nsec; -/* 36 */__le32 blocks; /* used for statfs */ -/* 40 */__le32 meta_blkaddr; -/* 44 */__le32 xattr_blkaddr; -/* 48 */__u8 uuid[16]; /* 128-bit uuid for volume */ -/* 64 */__u8 volume_name[16]; /* volume name */ -/* 80 */__le32 requirements;/* (aka. feature_incompat) */ - -/* 84 */__u8 reserved2[44]; -} __packed; /* 128 bytes */ + __le32 magic; /* file system magic number */ + __le32 checksum;/* crc32c(super_block) */ + __le32 features;/* (aka. feature_compat) */ + __u8 blkszbits; /* support block_size == PAGE_SIZE only */ + __u8 reserved; + + __le16 root_nid;/* nid of root directory */ + __le64 inos;/* total valid ino # (== f_files - f_favail) */ + + __le64 build_time; /* inode v1 time derivation */ + __le32 build_time_nsec; /* inode v1 time derivation in nano scale */ + __le32 blocks; /* used for statfs */ + __le32 meta_blkaddr;/* start block address of metadata area */ + __le32 xattr_blkaddr; /* start block address of shared xattr area */ + __u8 uuid[16]; /* 128-bit uuid for volume */ + __u8 volume_name[16]; /* volume name */ + __le32 requirements;/* (aka. feature_incompat) */ + + __u8 reserved2[44]; +} __packed; /* * erofs inode data mapping: @@ -73,16 +74,17 @@ static inline bool erofs_inode_is_data_compressed(unsigned int datamode) #define EROFS_I_VERSION_BIT 0 #define EROFS_I_DATA_MAPPING_BIT1 +/* 32-byte reduced form of an ondisk inode */ struct erofs_inode_v1 { -/* 0 */__le16 i_advise; + __le16 i_advise;/* inode hints */ /* 1 header + n-1 * 4 bytes inline xattr to keep continuity */ -/* 2 */__le16 i_xattr_icount; -/* 4 */__le16 i_mode; -/* 6 */__le16 i_nlink; -/* 8 */__le32 i_size; -/* 12 */__le32 i_reserved; -/* 16 */union { + __le16 i_xattr_icount; + __le16 i_mode; + __le16 i_nlink; + __le32 i_size; + __le32 i_reserved; + union { /* file total compressed blocks for data mapping 1 */ __le32 compressed_blocks; __le32 raw_blkaddr; @@ -90,10 +92,10 @@ struct erofs_inode_v1 { /* for device files, used to indicate old/new device # */ __le32 rdev; } i_u __packed; -/* 20 */__le32 i_ino; /* only used for 32-bit stat compatibility */ -/* 24 */__le16 i_uid; -/* 26 */__le16 i_gid; -/* 28 */__le32 i_reserved2; + __le32 i_ino; /* only used for 32-bit stat compatibility */ + __le16 i_uid; + __le16 i_gid; + __le32 i_reserved2; } __packed; /* 32 bytes on-disk inode */ @@ -101,15 +103,16 @@ struct erofs_inode_v1 { /* 64 bytes on-disk inode */ #define EROFS_INODE_LAYOUT_V2 1 +/* 64-byte complete form of an ondisk inode */ struct erofs_inode_v2 { -/* 0 */__le16 i_advise; + __le16 i_advise;/* inode hints */ /* 1 header + n-1 * 4 bytes inline xattr to keep continuity */ -/* 2 */__le16 i_xattr_icount; -/* 4 */__le16 i_mode; -/* 6 */__le16 i_reserved; -/* 8 */__le64 i_size; -/* 16 */union { + __le16 i_xattr_icount; + __le16 i_mode; + __le16 i_reserved; + __le64 i_size; + union { /* file total compressed blocks for data mapping 1 */ __le32 compressed_blocks; __le32 raw_blkaddr; @@ -119,15 +122,15 @@ struct erofs_inode_v2 { } i_u __packed; /* only used for 32-bit stat compatibility */ -/* 20 */__le32 i_ino; - -/* 24 */__le32 i_uid; -/* 28 */__le32 i_gid; -/* 32 */__le64 i_ctime; -/* 40 */__le32 i_ctime_ns
[PATCH 00/21] erofs: patchset addressing Christoph's comments
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. Comments and suggestions are welcome... [1] https://lore.kernel.org/r/1eed1e6b-f95e-aa8e-c3e7-e9870401e...@kernel.org/ Thanks, Gao Xiang Gao Xiang (21): 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: kill erofs_{init,exit}_inode_cache erofs: use erofs_inode naming erofs: update comments in inode.c erofs: update erofs symlink stuffs erofs: kill is_inode_layout_compression() erofs: use dsb instead of layout for ondisk super_block erofs: kill verbose debug info in erofs_fill_super erofs: simplify erofs_grab_bio() since bio_alloc() never fail erofs: kill prio and nofail of erofs_get_meta_page() erofs: kill __submit_bio() erofs: kill magic underscores erofs: use a switch statement when dealing with the file modes 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 Documentation/filesystems/erofs.txt | 9 -- fs/erofs/Kconfig| 7 -- fs/erofs/data.c | 62 +++--- fs/erofs/decompressor.c | 34 ++--- fs/erofs/dir.c | 6 +- fs/erofs/erofs_fs.h | 162 fs/erofs/inode.c| 176 +- fs/erofs/internal.h | 156 +++ fs/erofs/namei.c| 12 +- fs/erofs/super.c| 185 fs/erofs/xattr.c| 33 +++-- fs/erofs/xattr.h| 4 +- fs/erofs/zdata.c| 44 +++ fs/erofs/zmap.c | 32 ++--- include/trace/events/erofs.h| 14 +-- 15 files changed, 338 insertions(+), 598 deletions(-) -- 2.17.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v6 03/24] erofs: add super block operations
On Sat, Aug 31, 2019 at 09:34:44AM +0300, Amir Goldstein wrote: > On Fri, Aug 30, 2019 at 8:16 PM Gao Xiang wrote: > > > > Hi Christoph, > > > > On Fri, Aug 30, 2019 at 09:39:10AM -0700, Christoph Hellwig wrote: > > > On Thu, Aug 29, 2019 at 06:50:48PM +0800, Gao Xiang wrote: > > > > > Please use an erofs_ prefix for all your functions. > > > > > > > > It is already a static function, I have no idea what is wrong here. > > > > > > Which part of all wasn't clear? Have you looked at the prefixes for > > > most functions in the various other big filesystems? > > > > I will add erofs prefix to free_inode as you said. > > > > At least, all non-prefix functions in erofs are all static functions, > > it won't pollute namespace... I will add "erofs_" to other meaningful > > callbacks...And as you can see... > > > > cifs/cifsfs.c > > 1303:cifs_init_inodecache(void) > > 1509: rc = cifs_init_inodecache(); > > > > hpfs/super.c > > 254:static int init_inodecache(void) > > 771:int err = init_inodecache(); > > > > minix/inode.c > > 84:static int __init init_inodecache(void) > > 665:int err = init_inodecache(); > > > > Hi Gao, > > "They did it first" is never a good reply for code review comments. > Nobody cares if you copy code with init_inodecache(). > I understand why you thought static function names do not pollute > the (linker) namespace, but they do pollute the global namespace. > > free_inode() as a local function name is one of the worst examples > for VFS namespace pollution. > > VFS code uses function names like those a lot in the global namespace, e.g.: > clear_inode(),new_inode(). > > For example from recent history of namespace collision caused by your line > of thinking, see: > e6fd2093a85d md: namespace private helper names > > Besides, you really have nothing to loose from prefixing everything > with erofs_, do you? It's better for review, for debugging... Hi Amir, Thanks for you kind reply... Yes, I understand that some generic header files could have the same function names and cause bad behaviors... I will fix them, my only one question is "if all function/variable names are prefixed with "erofs_" (including all inline helpers in header files), it seems somewhat strange... (too many statements start "erofs_" in the source code...)" I will fix common and short names at once... Thanks, Gao Xiang > > Thanks, > Amir. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v6 03/24] erofs: add super block operations
Hi Christoph, On Sat, Aug 31, 2019 at 01:15:10AM +0800, Gao Xiang wrote: [] > > > > > > > + /* be careful RCU symlink path (see ext4_inode_info->i_data)! */ > > > > > + if (is_inode_fast_symlink(inode)) > > > > > + kfree(inode->i_link); > > > > > > > > is_inode_fast_symlink only shows up in a later patch. And really > > > > obsfucates the check here in the only caller as you can just do an > > > > unconditional kfree here - i_link will be NULL except for the case > > > > where you explicitly set it. > > > > > > I cannot fully understand your point (sorry about my English), > > > I will reply you about this later. > > > > With that I mean that you should: > > > > 1) remove is_inode_fast_symlink and just opencode it in the few places > > using it > > 2) remove the check in this place entirely as it is not needed Add some words about this suggestion since I'm addressing this place, it seems it could not (or I am not sure at least) be freed unconditionally union { struct pipe_inode_info *i_pipe; struct block_device *i_bdev; struct cdev *i_cdev; char*i_link; unsignedi_dir_seq; }; while I saw what shmem did, it seems that they handle as follows: 3636 static void shmem_free_in_core_inode(struct inode *inode) 3637 { 3638 if (S_ISLNK(inode->i_mode)) 3639 kfree(inode->i_link); 3640 kmem_cache_free(shmem_inode_cachep, SHMEM_I(inode)); 3641 } I think that would be some check on it to get it is a symlink (for i_dir_seq it seems unsafe) I think the original check is ok but I will opencode it instead. Thanks, Gao Xiang ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v6 05/24] erofs: add inode operations
Hi Christoph, On Fri, Aug 30, 2019 at 09:42:05AM -0700, Christoph Hellwig wrote: > On Thu, Aug 29, 2019 at 07:59:22PM +0800, Gao Xiang wrote: > > On Thu, Aug 29, 2019 at 03:24:26AM -0700, Christoph Hellwig wrote: > > > > [] > > > > > > > > > + > > > > + /* fill last page if inline data is available */ > > > > + err = fill_inline_data(inode, data, ofs); > > > > > > Well, I think you should move the is_inode_flat_inline and > > > (S_ISLNK(inode->i_mode) && inode->i_size < PAGE_SIZE) checks from that > > > helper here, as otherwise you make everyone wonder why you'd always > > > fill out the inline data. > > > > Currently, fill_inline_data() only fills for fast symlink, > > later we can fill any tail-end block (such as dir block) > > for our requirements. > > So change it when that later changes actually come in. And even then > having the checks outside the function is a lot more obvious. Okay. > > > And I think that is minor. > > The problem is that each of these issues might appear minor on their > own. But combined a lot of the coding style choices lead to code that > is more suitable an obsfucated code contest than the Linux kernel as > trying to understand even just a few places requires jumping through > tons of helpers with misleading names and spread over various files. > > > The consideration is simply because iget_locked performs better > > than iget5_locked. > > In what benchmark do the differences show up? In a word, no benchmark here, just because "unsigned long on 32-bit platforms is 4 bytes." but erofs nid is a 64-bit number. iget_locked will do find_inode_fast (no callback at all) rather than iget5_locked --> find_inode (test callback) -> inode_insert5(set callback) for each new inode. For most 64-bit platforms, iget_locked is enough, 32-bit platforms become rare... Thanks, Gao Xiang ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v6 04/24] erofs: add raw address_space operations
Hi Christoph, On Fri, Aug 30, 2019 at 09:40:13AM -0700, Christoph Hellwig wrote: > On Thu, Aug 29, 2019 at 07:46:11PM +0800, Gao Xiang wrote: > > Hi Christoph, > > > > On Thu, Aug 29, 2019 at 03:17:21AM -0700, Christoph Hellwig wrote: > > > The actual address_space operations seem to largely duplicate > > > the iomap versions. Please use those instead. Also I don't think > > > any new file system should write up ->bmap these days. > > > > iomap doesn't support tail-end packing inline data till now, > > I think Chao and I told you and Andreas before [1]. > > > > Since EROFS keeps a self-contained driver for now, we will use > > iomap if it supports tail-end packing inline data later. > > Well, so work with the maintainers to enhance the core kernel. That > is how Linux development works. We've added various iomap enhancements > for gfs in the last merge windows, and we've added more for the brand > new zonefs file system we plan to merge for 5.4. That is a good idea, I think Chao will continue working on this (adding tail-end packing inline approach into iomap, thus we can have few code in data.c.) Thanks, Gao Xiang ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v6 03/24] erofs: add super block operations
Hi Christoph, On Fri, Aug 30, 2019 at 09:39:10AM -0700, Christoph Hellwig wrote: > On Thu, Aug 29, 2019 at 06:50:48PM +0800, Gao Xiang wrote: > > > Please use an erofs_ prefix for all your functions. > > > > It is already a static function, I have no idea what is wrong here. > > Which part of all wasn't clear? Have you looked at the prefixes for > most functions in the various other big filesystems? I will add erofs prefix to free_inode as you said. At least, all non-prefix functions in erofs are all static functions, it won't pollute namespace... I will add "erofs_" to other meaningful callbacks...And as you can see... cifs/cifsfs.c 1303:cifs_init_inodecache(void) 1509: rc = cifs_init_inodecache(); hpfs/super.c 254:static int init_inodecache(void) 771:int err = init_inodecache(); minix/inode.c 84:static int __init init_inodecache(void) 665:int err = init_inodecache(); isofs/inode.c 88:static int __init init_inodecache(void) 1580: int err = init_inodecache(); bfs/inode.c 261:static int __init init_inodecache(void) 468:int err = init_inodecache(); ext4/super.c 1144:static int __init init_inodecache(void) 6115: err = init_inodecache(); reiserfs/super.c 666:static int __init init_inodecache(void) 2606: ret = init_inodecache(); squashfs/super.c 406:static int __init init_inodecache(void) 430:int err = init_inodecache(); udf/super.c 177:static int __init init_inodecache(void) 232:err = init_inodecache(); qnx4/inode.c 358:static int init_inodecache(void) 399:err = init_inodecache(); ufs/super.c 1463:static int __init init_inodecache(void) 1517: int err = init_inodecache(); qnx6/inode.c 618:static int init_inodecache(void) 659:err = init_inodecache(); f2fs/super.c 3540:static int __init init_inodecache(void) 3572: err = init_inodecache(); > > > > > + /* be careful RCU symlink path (see ext4_inode_info->i_data)! */ > > > > + if (is_inode_fast_symlink(inode)) > > > > + kfree(inode->i_link); > > > > > > is_inode_fast_symlink only shows up in a later patch. And really > > > obsfucates the check here in the only caller as you can just do an > > > unconditional kfree here - i_link will be NULL except for the case > > > where you explicitly set it. > > > > I cannot fully understand your point (sorry about my English), > > I will reply you about this later. > > With that I mean that you should: > > 1) remove is_inode_fast_symlink and just opencode it in the few places > using it > 2) remove the check in this place entirely as it is not needed > 3) remove the comment quoted above as it is more confusing than not > having the comment Got it, thanks! > > > > Is there any good reasons to use buffer heads like this in new code > > > vs directly using bios? > > > > This page can save in bdev page cache, it contains not only the erofs > > superblock so it can be fetched in page cache later. > > If you want it in the page cache why not use read_mapping_page or similar? It's reasonable, I will change as you suggested. (The difference is whether it has some buffer_head to the sb page or not...) > > > > > +/* set up default EROFS parameters */ > > > > +static void default_options(struct erofs_sb_info *sbi) > > > > +{ > > > > +} > > > > > > No need to add an empty function. > > > > Later patch will fill this function. > > Please only add the function in the patch actually adding the > functionality. That was my fault when spilting patches...considering it's an >7KLOC filesystem (maybe spilting the whole xfs or ext4 properly is more harder)... Anyway, that is my fault. > > > > > +} > > > > > > Why is this needed? You can just free your sb privatte information in > > > ->put_super and wire up kill_block_super as the ->kill_sb method > > > directly. > > > > See Al's comments, > > https://lore.kernel.org/r/20190720224955.gd17...@zeniv.linux.org.uk/ > > With that code it makes sense. In this paticular patch it does not. > So please add it only when actually needed. Same as above... Thanks, Gao Xiang ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v8 20/24] erofs: introduce generic decompression backend
Hi Christoph, On Fri, Aug 30, 2019 at 09:35:34AM -0700, Christoph Hellwig wrote: > On Thu, Aug 15, 2019 at 12:41:51PM +0800, Gao Xiang wrote: > > +static bool use_vmap; > > +module_param(use_vmap, bool, 0444); > > +MODULE_PARM_DESC(use_vmap, "Use vmap() instead of vm_map_ram() (default > > 0)"); > > And how would anyone know which to pick? It has significant FIO benchmark difference on sequential read least on arm64... I have no idea whether all platform vm_map_ram() behaves better than vmap(), so I leave an option for users here... Thanks, Gao Xiang ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 7/7] erofs: redundant assignment in __erofs_get_meta_page()
Hi Christoph, On Fri, Aug 30, 2019 at 09:28:12AM -0700, Christoph Hellwig wrote: > > - err = bio_add_page(bio, page, PAGE_SIZE, 0); > > - if (err != PAGE_SIZE) { > > + if (bio_add_page(bio, page, PAGE_SIZE, 0) != PAGE_SIZE) { > > err = -EFAULT; > > goto err_out; > > } > > This patch looks like an improvement. But looking at that whole > area just makes me cringe. OK, I agree with you, I will improve it or just kill them all with new iomap approach after it supports tail-end packing inline. > > Why is there __erofs_get_meta_page with the two weird booleans instead > of a single erofs_get_meta_page that gets and gfp_t for additional > flags and an unsigned int for additional bio op flags. I agree with you. Thanks for your suggestion. > > Why do need ioprio support to start with? Seeing that in a new > fs look kinda odd. Do you have benchmarks that show the difference? I don't have some benchmark for all of these, can I just set REQ_PRIO for all metadata? is that reasonable? Could you kindly give some suggestion on this? > > That function then calls erofs_grab_bio, which tries to handle a > bio_alloc failure, except that the function will not actually fail > due the mempool backing it. It also seems like and awfully > huge function to inline. OK, I will simplify it. Thanks for your suggestion. > > Why is there __submit_bio which really just obsfucates what is > going on? Also why is __submit_bio using bio_set_op_attrs instead > of opencode it as the comment right next to it asks you to? Originally, mainly due to backport consideration since some of our smartphones use 3.x kernel as well... > > Also I really don't understand why you can't just use read_cache_page > or even read_cache_page_gfp instead of __erofs_get_meta_page. > That function is a whole lot of duplication of functionality shared > by a lot of other file systems. OK, I have to admit, that code was originally just copied from f2fs with some modification (maybe it's not a good example for us). Thanks, Gao Xiang ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 6/7] erofs: remove all likely/unlikely annotations
Hi Christoph, On Fri, Aug 30, 2019 at 08:46:50AM -0700, Christoph Hellwig wrote: > On Fri, Aug 30, 2019 at 11:36:42AM +0800, Gao Xiang wrote: > > As Dan Carpenter suggested [1], I have to remove > > all erofs likely/unlikely annotations. > > Do you have to remove all of them, or just those where you don't have > a particularly good reason why you think in this particular case they > might actually matter? I just added unlikely/likely for all erofs error handling paths or rare happened cases at first... (That is all in my thought...) I don't have some benchmark data for each unlikely/likely case (and I have no idea "is that worth to take time to benchmark rather than do another more useful stuffs"), so..I have to kill them all... Thanks, Gao Xiang ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 2/7] erofs: some marcos are much more readable as a function
On Fri, Aug 30, 2019 at 11:52:23PM +0800, Gao Xiang wrote: > Hi Christoph, > > On Fri, Aug 30, 2019 at 08:45:51AM -0700, Christoph Hellwig wrote: > > On Thu, Aug 29, 2019 at 08:16:27PM -0700, Joe Perches wrote: > > > > - sizeof(__u32) * ((__count) - 1); }) > > > > +static inline unsigned int erofs_xattr_ibody_size(__le16 d_icount) > > > > +{ > > > > + unsigned int icount = le16_to_cpu(d_icount); > > > > + > > > > + if (!icount) > > > > + return 0; > > > > + > > > > + return sizeof(struct erofs_xattr_ibody_header) + > > > > + sizeof(__u32) * (icount - 1); > > > > > > Maybe use struct_size()? > > > > Declaring a variable that is only used for struct_size is rather ugly. > > But while we are nitpicking: you don't need to byteswap to check for 0, > > so the local variable could be avoided. > > > > Also what is that magic -1 for? Normally we use that for the > > deprecated style where a variable size array is declared using > > varname[1], but that doesn't seem to be the case for erofs. > > I have to explain more about this (sorry about my awkward English) > here i_xattr_icount is to represent the size of xattr field of erofs, as > follows: > 0 - no xattr at all (no erofs_xattr_ibody_header) > ___ > | inode | > |___| > > 1 - a erofs_xattr_ibody_header (12 byte) + 4-byte (shared + inline) xattrs > 2 - a erofs_xattr_ibody_header (12 byte) + 8-byte (shared + inline) xattrs > > (that is the magic -1 means...) > > In order to keep the number continuously, actually the content could be > an array of shared_xattr_id and > an inline xattr combination (struct erofs_xattr_entry + name + value) ...Add a word, large xattrs should use shared xattr (which save xattrs in another area) rather than inline xattr, shared xattr stores xattr_id just after erofs_xattr_ibody_header and before inline xattrs... Thanks, Gao Xiang > > Thanks, > Gao Xiang > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 2/7] erofs: some marcos are much more readable as a function
Hi Christoph, On Fri, Aug 30, 2019 at 08:45:51AM -0700, Christoph Hellwig wrote: > On Thu, Aug 29, 2019 at 08:16:27PM -0700, Joe Perches wrote: > > > - sizeof(__u32) * ((__count) - 1); }) > > > +static inline unsigned int erofs_xattr_ibody_size(__le16 d_icount) > > > +{ > > > + unsigned int icount = le16_to_cpu(d_icount); > > > + > > > + if (!icount) > > > + return 0; > > > + > > > + return sizeof(struct erofs_xattr_ibody_header) + > > > + sizeof(__u32) * (icount - 1); > > > > Maybe use struct_size()? > > Declaring a variable that is only used for struct_size is rather ugly. > But while we are nitpicking: you don't need to byteswap to check for 0, > so the local variable could be avoided. > > Also what is that magic -1 for? Normally we use that for the > deprecated style where a variable size array is declared using > varname[1], but that doesn't seem to be the case for erofs. I have to explain more about this (sorry about my awkward English) here i_xattr_icount is to represent the size of xattr field of erofs, as follows: 0 - no xattr at all (no erofs_xattr_ibody_header) ___ | inode | |___| 1 - a erofs_xattr_ibody_header (12 byte) + 4-byte (shared + inline) xattrs 2 - a erofs_xattr_ibody_header (12 byte) + 8-byte (shared + inline) xattrs (that is the magic -1 means...) In order to keep the number continuously, actually the content could be an array of shared_xattr_id and an inline xattr combination (struct erofs_xattr_entry + name + value) Thanks, Gao Xiang ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 1/7] erofs: on-disk format should have explicitly assigned numbers
On Fri, Aug 30, 2019 at 11:36:37AM +0800, Gao Xiang wrote: > As Christoph claimed [1], on-disk format should have > explicitly assigned numbers. I have to change it. > > [1] https://lore.kernel.org/r/20190829095954.gb20...@infradead.org/ > Reported-by: Christoph Hellwig > Reviewed-by: Chao Yu > Signed-off-by: Gao Xiang ...ignore this patchset. I will resend another incremental patchset to address what Christoph suggested yesterday... Thanks, Gao Xiang > --- > no change > > fs/erofs/erofs_fs.h | 18 +- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/fs/erofs/erofs_fs.h b/fs/erofs/erofs_fs.h > index afa7d45ca958..2447ad4d0920 100644 > --- a/fs/erofs/erofs_fs.h > +++ b/fs/erofs/erofs_fs.h > @@ -52,10 +52,10 @@ struct erofs_super_block { > * 4~7 - reserved > */ > enum { > - EROFS_INODE_FLAT_PLAIN, > - EROFS_INODE_FLAT_COMPRESSION_LEGACY, > - EROFS_INODE_FLAT_INLINE, > - EROFS_INODE_FLAT_COMPRESSION, > + EROFS_INODE_FLAT_PLAIN = 0, > + EROFS_INODE_FLAT_COMPRESSION_LEGACY = 1, > + EROFS_INODE_FLAT_INLINE = 2, > + EROFS_INODE_FLAT_COMPRESSION= 3, > EROFS_INODE_LAYOUT_MAX > }; > > @@ -181,7 +181,7 @@ struct erofs_xattr_entry { > > /* available compression algorithm types */ > enum { > - Z_EROFS_COMPRESSION_LZ4, > + Z_EROFS_COMPRESSION_LZ4 = 0, > Z_EROFS_COMPRESSION_MAX > }; > > @@ -239,10 +239,10 @@ struct z_erofs_map_header { > *(di_advise could be 0, 1 or 2) > */ > enum { > - Z_EROFS_VLE_CLUSTER_TYPE_PLAIN, > - Z_EROFS_VLE_CLUSTER_TYPE_HEAD, > - Z_EROFS_VLE_CLUSTER_TYPE_NONHEAD, > - Z_EROFS_VLE_CLUSTER_TYPE_RESERVED, > + Z_EROFS_VLE_CLUSTER_TYPE_PLAIN = 0, > + Z_EROFS_VLE_CLUSTER_TYPE_HEAD = 1, > + Z_EROFS_VLE_CLUSTER_TYPE_NONHEAD= 2, > + Z_EROFS_VLE_CLUSTER_TYPE_RESERVED = 3, > Z_EROFS_VLE_CLUSTER_TYPE_MAX > }; > > -- > 2.17.1 > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] erofs: using switch-case while checking the inode type.
On Fri, Aug 30, 2019 at 07:59:48PM +0800, Gao Xiang wrote: > Hi Pratik, > > The subject line could be better as '[PATCH v2] xx'... > > On Fri, Aug 30, 2019 at 03:26:15PM +0530, Pratik Shinde wrote: > > while filling the linux inode, using switch-case statement to check > > the type of inode. > > switch-case statement looks more clean here. > > > > Signed-off-by: Pratik Shinde > > I have no problem of this patch, and I will do a test and reply > you "Reviewed-by:" in hours (I'm doing another patchset to resolve > what Christoph suggested again)... Reviewed-by: Gao Xiang Thanks, Gao Xiang > > Thanks, > Gao Xiang > > > --- > > fs/erofs/inode.c | 18 -- > > 1 file changed, 12 insertions(+), 6 deletions(-) > > > > diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c > > index 80f4fe9..6336cc1 100644 > > --- a/fs/erofs/inode.c > > +++ b/fs/erofs/inode.c > > @@ -190,22 +190,28 @@ static int fill_inode(struct inode *inode, int isdir) > > err = read_inode(inode, data + ofs); > > if (!err) { > > /* setup the new inode */ > > - if (S_ISREG(inode->i_mode)) { > > + switch (inode->i_mode & S_IFMT) { > > + case S_IFREG: > > inode->i_op = _generic_iops; > > inode->i_fop = _ro_fops; > > - } else if (S_ISDIR(inode->i_mode)) { > > + break; > > + case S_IFDIR: > > inode->i_op = _dir_iops; > > inode->i_fop = _dir_fops; > > - } else if (S_ISLNK(inode->i_mode)) { > > + break; > > + case S_IFLNK: > > /* by default, page_get_link is used for symlink */ > > inode->i_op = _symlink_iops; > > inode_nohighmem(inode); > > - } else if (S_ISCHR(inode->i_mode) || S_ISBLK(inode->i_mode) || > > - S_ISFIFO(inode->i_mode) || S_ISSOCK(inode->i_mode)) { > > + break; > > + case S_IFCHR: > > + case S_IFBLK: > > + case S_IFIFO: > > + case S_IFSOCK: > > inode->i_op = _generic_iops; > > init_special_inode(inode, inode->i_mode, inode->i_rdev); > > goto out_unlock; > > - } else { > > + default: > > err = -EFSCORRUPTED; > > goto out_unlock; > > } > > -- > > 2.9.3 > > > > ___ > > devel mailing list > > de...@linuxdriverproject.org > > http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v6 01/24] erofs: add on-disk layout
Hi David, On Fri, Aug 30, 2019 at 02:07:14PM +0200, David Sterba wrote: > On Thu, Aug 29, 2019 at 08:58:17AM -0700, Joe Perches wrote: > > On Thu, 2019-08-29 at 18:32 +0800, Gao Xiang wrote: > > > Hi Christoph, > > > > > > On Thu, Aug 29, 2019 at 02:59:54AM -0700, Christoph Hellwig wrote: > > > > > --- /dev/null > > > > > +++ b/fs/erofs/erofs_fs.h > > > > > @@ -0,0 +1,316 @@ > > > > > +/* SPDX-License-Identifier: GPL-2.0-only OR Apache-2.0 */ > > > > > +/* > > > > > + * linux/fs/erofs/erofs_fs.h > > > > > > > > Please remove the pointless file names in the comment headers. > > > > > > Already removed in the latest version. > > > > > > > > +struct erofs_super_block { > > > > > +/* 0 */__le32 magic; /* in the little endian */ > > > > > +/* 4 */__le32 checksum;/* crc32c(super_block) */ > > > > > +/* 8 */__le32 features;/* (aka. feature_compat) */ > > > > > +/* 12 */__u8 blkszbits; /* support block_size == PAGE_SIZE > > > > > only */ > > > > > > > > Please remove all the byte offset comments. That is something that can > > > > easily be checked with gdb or pahole. > > > > > > I have no idea the actual issue here. > > > It will help all developpers better add fields or calculate > > > these offsets in their mind, and with care. > > > > > > Rather than they didn't run "gdb" or "pahole" and change it by mistake. > > > > I think Christoph is not right here. > > > > Using external tools for validation is extra work > > when necessary for understanding the code. > > The advantage of using the external tools that the information about > offsets is provably correct ... > > > The expected offset is somewhat valuable, but > > perhaps the form is a bit off given the visual > > run-in to the field types. > > > > The extra work with this form is manipulating all > > the offsets whenever a structure change occurs. > > ... while this is error prone. I will redo a full patchset and comments addressing what Christoph all said yesterday. Either form is fine with me for this case, let's remove them instead. Thanks, Gao Xiang > > > The comments might be better with a form more like: > > > > struct erofs_super_block { /* offset description */ > > __le32 magic; /* 0 */ > > __le32 checksum;/* 4 crc32c(super_block) */ > > __le32 features;/* 8 (aka. feature_compat) */ > > __u8 blkszbits; /* 12 support block_size == PAGE_SIZE only */ ___ 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
Hi Dan, On Fri, Aug 30, 2019 at 02:30:47PM +0300, Dan Carpenter wrote: > On Fri, Aug 30, 2019 at 11:36:42AM +0800, Gao Xiang wrote: > > As Dan Carpenter suggested [1], I have to remove > > all erofs likely/unlikely annotations. > > > > [1] https://lore.kernel.org/linux-fsdevel/20190829154346.GK23584@kadam/ > > Reported-by: Dan Carpenter > > Signed-off-by: Gao Xiang > > --- > > Thanks! > > This is a nice readability improvement and I'm so sure it won't impact > benchmarking at all. > > Acked-by: Dan Carpenter It seems Greg merged another version... I have no idea but thanks for your acked-by :) https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/commit/?h=staging-testing=8d8a09b093d7073465c824f74caf315c073d3875 THanks, Gao Xiang > > regards, > dan carpenter > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: exfat: add exfat filesystem code to staging
Hi Dan, On Fri, Aug 30, 2019 at 02:26:12PM +0300, Dan Carpenter wrote: > On Fri, Aug 30, 2019 at 04:43:33PM +0800, Gao Xiang wrote: > > Hi Dan, > > > > On Fri, Aug 30, 2019 at 11:34:45AM +0300, Dan Carpenter wrote: > > > On Fri, Aug 30, 2019 at 12:04:41AM +0800, Gao Xiang wrote: > > > > Anyway, I'm fine to delete them all if you like, but I think majority > > > > of these > > > > are meaningful. > > > > > > > > data.c- /* page is already locked */ > > > > data.c- DBG_BUGON(PageUptodate(page)); > > > > data.c- > > > > data.c: if (unlikely(err)) > > > > data.c- SetPageError(page); > > > > data.c- else > > > > data.c- SetPageUptodate(page); > > > > > > If we cared about speed here then we would delete the DBG_BUGON() check > > > because that's going to be expensive. The likely/unlikely annotations > > > should be used in places a reasonable person thinks it will make a > > > difference to benchmarks. > > > > DBG_BUGON will be a no-op ((void)x) in non-debugging mode, > > It expands to: > > ((void)PageUptodate(page)); > > Calling PageUptodate() doesn't do anything, but it isn't free. The > time it takes to do that function call completely negates any speed up > from using likely/unlikely. > > I'm really not trying to be a jerk... You are right, I recalled that PageUptodate is not as simple as it implys. Yes, those are all removed now... I am ok with that, thanks for your suggestion :) Thanks, Gao Xiang > > regards, > dan carpenter > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] erofs: using switch-case while checking the inode type.
Hi Pratik, The subject line could be better as '[PATCH v2] xx'... On Fri, Aug 30, 2019 at 03:26:15PM +0530, Pratik Shinde wrote: > while filling the linux inode, using switch-case statement to check > the type of inode. > switch-case statement looks more clean here. > > Signed-off-by: Pratik Shinde I have no problem of this patch, and I will do a test and reply you "Reviewed-by:" in hours (I'm doing another patchset to resolve what Christoph suggested again)... Thanks, Gao Xiang > --- > fs/erofs/inode.c | 18 -- > 1 file changed, 12 insertions(+), 6 deletions(-) > > diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c > index 80f4fe9..6336cc1 100644 > --- a/fs/erofs/inode.c > +++ b/fs/erofs/inode.c > @@ -190,22 +190,28 @@ static int fill_inode(struct inode *inode, int isdir) > err = read_inode(inode, data + ofs); > if (!err) { > /* setup the new inode */ > - if (S_ISREG(inode->i_mode)) { > + switch (inode->i_mode & S_IFMT) { > + case S_IFREG: > inode->i_op = _generic_iops; > inode->i_fop = _ro_fops; > - } else if (S_ISDIR(inode->i_mode)) { > + break; > + case S_IFDIR: > inode->i_op = _dir_iops; > inode->i_fop = _dir_fops; > - } else if (S_ISLNK(inode->i_mode)) { > + break; > + case S_IFLNK: > /* by default, page_get_link is used for symlink */ > inode->i_op = _symlink_iops; > inode_nohighmem(inode); > - } else if (S_ISCHR(inode->i_mode) || S_ISBLK(inode->i_mode) || > - S_ISFIFO(inode->i_mode) || S_ISSOCK(inode->i_mode)) { > + break; > + case S_IFCHR: > + case S_IFBLK: > + case S_IFIFO: > + case S_IFSOCK: > inode->i_op = _generic_iops; > init_special_inode(inode, inode->i_mode, inode->i_rdev); > goto out_unlock; > - } else { > + default: > err = -EFSCORRUPTED; > goto out_unlock; > } > -- > 2.9.3 > > ___ > devel mailing list > de...@linuxdriverproject.org > http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: exfat: add exfat filesystem code to staging
Hi Dan, On Fri, Aug 30, 2019 at 11:34:45AM +0300, Dan Carpenter wrote: > On Fri, Aug 30, 2019 at 12:04:41AM +0800, Gao Xiang wrote: > > Anyway, I'm fine to delete them all if you like, but I think majority of > > these > > are meaningful. > > > > data.c- /* page is already locked */ > > data.c- DBG_BUGON(PageUptodate(page)); > > data.c- > > data.c: if (unlikely(err)) > > data.c- SetPageError(page); > > data.c- else > > data.c- SetPageUptodate(page); > > If we cared about speed here then we would delete the DBG_BUGON() check > because that's going to be expensive. The likely/unlikely annotations > should be used in places a reasonable person thinks it will make a > difference to benchmarks. DBG_BUGON will be a no-op ((void)x) in non-debugging mode, I discussed related stuffs with Greg many months before [1] and other filesystems also have similar functions... p.s. I think we come to an agreement here... I killed all unlikely/likely. [1] http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2018-November/128951.html sorry about no lore here. Thanks, Gao Xiang > > regards, > dan carpenter > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: exfat: add exfat filesystem code to staging
Hi Dan, 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. I'm also curious about that, what is the filesystem or kernel standard about likely/unlikely use (since I didn't find some documented standard so I used in my personal way, I think it is reasonable at least to cover all error handling paths), maybe I'm an _idiot_ as some earlier unfriendly word said somewhere so I'm too stupid to understand the implicit meaning of some document. > > 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. Add a word (just a note), I don't think such kind of "any", "few", "all" words are meaningful without some explicit evidence. (e.g. such as EROFS have few error handling path. I don't think that is true and worth to give many details since EROFS code is here for more than one year.) Yes, EROFS is not prefectly, I have to admit, and I said similar words on other threads for many times if you decide to check each likely/unlikely line by line, I cannot say all unlikely/likely cases I wrote are reasonable (just as bug-free, I think no one can make such guarantee even for new code), but I can say the majority of them are reasonable in my personal understanding of likely/unlikely. And I can fix all your reports in time (but maybe some are not urgent at all.) In addition there will be endless discussions on detailed code since there are many personal tendencies from various people in it, as the saying goes "There are a thousand Hamlets in a thousand people's eyes. " Anyway, I have sent a patch to kill them all blindly as you like, so I think we can come to an agreement on it, but I still don't fully agree with your "for EROFS, I don't think that any of the likely/unlikely calls have been thought about" conclusion. Thanks, Gao Xiang > > > > regards, > > dan carpenter > > > > . > > ___ 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
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 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
[PATCH v4 2/7] erofs: some macros are much more readable as a function
As Christoph suggested [1], these macros 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 --- v4: a type fix in commit message. v3: change as Joe suggested, https://lore.kernel.org/r/5b2ecf5cec1a6aa3834e9af41886a7fcb18ae86a.ca...@perches.com/ fs/erofs/erofs_fs.h | 24 fs/erofs/inode.c| 4 ++-- fs/erofs/xattr.c| 2 +- 3 files changed, 19 insertions(+), 11 deletions(-) diff --git a/fs/erofs/erofs_fs.h b/fs/erofs/erofs_fs.h index 2447ad4d0920..0782ba9da623 100644 --- a/fs/erofs/erofs_fs.h +++ b/fs/erofs/erofs_fs.h @@ -168,16 +168,24 @@ struct erofs_xattr_entry { char e_name[0]; /* attribute name */ } __packed; -#define ondisk_xattr_ibody_size(count) ({\ - u32 __count = le16_to_cpu(count); \ - ((__count) == 0) ? 0 : \ - sizeof(struct erofs_xattr_ibody_header) + \ - sizeof(__u32) * ((__count) - 1); }) +static inline unsigned int erofs_xattr_ibody_size(__le16 d_icount) +{ + struct erofs_xattr_ibody_header *ibh; + unsigned int icount = le16_to_cpu(d_icount); + + if (!icount) + return 0; + + return struct_size(ibh, h_shared_xattrs, icount - 1); +} #define EROFS_XATTR_ALIGN(size) round_up(size, sizeof(struct erofs_xattr_entry)) -#define EROFS_XATTR_ENTRY_SIZE(entry) EROFS_XATTR_ALIGN( \ - sizeof(struct erofs_xattr_entry) + \ - (entry)->e_name_len + le16_to_cpu((entry)->e_value_size)) + +static inline unsigned int erofs_xattr_entry_size(struct erofs_xattr_entry *e) +{ + return EROFS_XATTR_ALIGN(sizeof(struct erofs_xattr_entry) + +e->e_name_len + le16_to_cpu(e->e_value_size)); +} /* available compression algorithm types */ enum { diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c index 80f4fe919ee7..cf31554075c9 100644 --- a/fs/erofs/inode.c +++ b/fs/erofs/inode.c @@ -29,7 +29,7 @@ static int read_inode(struct inode *inode, void *data) struct erofs_inode_v2 *v2 = data; vi->inode_isize = sizeof(struct erofs_inode_v2); - vi->xattr_isize = ondisk_xattr_ibody_size(v2->i_xattr_icount); + vi->xattr_isize = erofs_xattr_ibody_size(v2->i_xattr_icount); inode->i_mode = le16_to_cpu(v2->i_mode); if (S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode) || @@ -62,7 +62,7 @@ static int read_inode(struct inode *inode, void *data) struct erofs_sb_info *sbi = EROFS_SB(inode->i_sb); vi->inode_isize = sizeof(struct erofs_inode_v1); - vi->xattr_isize = ondisk_xattr_ibody_size(v1->i_xattr_icount); + vi->xattr_isize = erofs_xattr_ibody_size(v1->i_xattr_icount); inode->i_mode = le16_to_cpu(v1->i_mode); if (S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode) || diff --git a/fs/erofs/xattr.c b/fs/erofs/xattr.c index a8286998a079..7ef8d4bb45cd 100644 --- a/fs/erofs/xattr.c +++ b/fs/erofs/xattr.c @@ -231,7 +231,7 @@ static int xattr_foreach(struct xattr_iter *it, */ entry = *(struct erofs_xattr_entry *)(it->kaddr + it->ofs); if (tlimit) { - unsigned int entry_sz = EROFS_XATTR_ENTRY_SIZE(); + unsigned int entry_sz = erofs_xattr_entry_size(); /* xattr on-disk corruption: xattr entry beyond xattr_isize */ if (unlikely(*tlimit < entry_sz)) { -- 2.17.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v3 4/7] erofs: kill __packed for on-disk structures
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 --- no change. fs/erofs/erofs_fs.h | 18 +- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/fs/erofs/erofs_fs.h b/fs/erofs/erofs_fs.h index fbdaf873d736..b07984a17f11 100644 --- a/fs/erofs/erofs_fs.h +++ b/fs/erofs/erofs_fs.h @@ -37,7 +37,7 @@ struct erofs_super_block {/* off description */ __le32 requirements;/* 80 (aka. feature_incompat) */ __u8 reserved2[44]; /* 84 */ -} __packed;/* 128 bytes */ +}; /* 128 bytes */ /* * erofs inode data mapping: @@ -89,12 +89,12 @@ struct erofs_inode_v1 { /* off description */ /* for device files, used to indicate old/new device # */ __le32 rdev; - } i_u __packed; + } i_u; __le32 i_ino; /* 20 only used for 32-bit stat compatibility */ __le16 i_uid; /* 24 */ __le16 i_gid; /* 26 */ __le32 i_reserved2; /* 28 */ -} __packed;/* 32 bytes */ +}; /* 32 bytes */ /* 32 bytes on-disk inode */ #define EROFS_INODE_LAYOUT_V1 0 @@ -116,7 +116,7 @@ struct erofs_inode_v2 { /* off description */ /* for device files, used to indicate old/new device # */ __le32 rdev; - } i_u __packed; + } i_u; /* only used for 32-bit stat compatibility */ __le32 i_ino; /* 20 only used for 32-bit stat compatibility */ @@ -127,7 +127,7 @@ struct erofs_inode_v2 { /* off description */ __le32 i_ctime_nsec;/* 40 */ __le32 i_nlink; /* 44 */ __u8 i_reserved2[16]; /* 48 */ -} __packed;/* 64 bytes */ +}; /* 64 bytes */ #define EROFS_MAX_SHARED_XATTRS (128) /* h_shared_count between 129 ... 255 are special # */ @@ -149,7 +149,7 @@ struct erofs_xattr_ibody_header { __u8 h_shared_count; __u8 h_reserved2[7]; __le32 h_shared_xattrs[0]; /* shared xattr id array */ -} __packed; +}; /* Name indexes */ #define EROFS_XATTR_INDEX_USER 1 @@ -166,7 +166,7 @@ struct erofs_xattr_entry { __le16 e_value_size;/* size of attribute value */ /* followed by e_name and e_value */ char e_name[0]; /* attribute name */ -} __packed; +}; static inline unsigned int erofs_xattr_ibody_size(__le16 d_icount) { @@ -272,8 +272,8 @@ struct z_erofs_vle_decompressed_index { * [1] - pointing to the tail cluster */ __le16 delta[2]; - } di_u __packed;/* 8 bytes */ -} __packed; + } di_u; /* 8 bytes */ +}; #define Z_EROFS_VLE_LEGACY_INDEX_ALIGN(size) \ (round_up(size, sizeof(struct z_erofs_vle_decompressed_index)) + \ -- 2.17.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel