[v2,00/25] erofs: patchset addressing Christoph's comments
mbox series

Message ID 20190904020912.63925-1-gaoxiang25@huawei.com
Headers show
Series
  • erofs: patchset addressing Christoph's comments
Related show

Message

Gao Xiang Sept. 4, 2019, 2:08 a.m. UTC
Hi,

This patchset is based on the following patch by Pratik Shinde,
https://lore.kernel.org/linux-erofs/20190830095615.10995-1-pratikshinde320@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-e9870401ee23@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.GL15931@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(-)

Comments

Chao Yu Sept. 4, 2019, 3:27 a.m. UTC | #1
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-pratikshinde320@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 <yuchao0@huawei.com>

Thanks,

> 
> Comments and suggestions are welcome...
> 
> [1] https://lore.kernel.org/r/1eed1e6b-f95e-aa8e-c3e7-e9870401ee23@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.GL15931@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(-)
>
Christoph Hellwig Sept. 4, 2019, 5:16 a.m. UTC | #2
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-pratikshinde320@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?
Gao Xiang Sept. 4, 2019, 6:08 a.m. UTC | #3
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-pratikshinde320@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
Gao Xiang Sept. 5, 2019, 1:03 a.m. UTC | #4
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-pratikshinde320@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 <yuchao0@huawei.com>
> 
> 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-e9870401ee23@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.GL15931@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
> devel@linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Christoph Hellwig Sept. 5, 2019, 11:30 a.m. UTC | #5
On Thu, Sep 05, 2019 at 09:03:54AM +0800, Gao Xiang wrote:
> 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...

Yes, I'm fine with these patches.