Hi!
> > +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 ==
Hi!
> > > 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
On Sun, Sep 01, 2019 at 01:51:11PM +0800, Gao Xiang wrote:
> enum {
> - EROFS_INODE_FLAT_PLAIN,
> - EROFS_INODE_FLAT_COMPRESSION_LEGACY,
> - EROFS_INODE_FLAT_INLINE,
> - EROFS_INODE_FLAT_COMPRESSION,
> + EROFS_INODE_FLAT_PLAIN = 0,
> +
On Sun, Sep 01, 2019 at 01:51:10PM +0800, Gao Xiang wrote:
> 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."
Looks good. If you want to keep them after the field names as someone
Thanks,
this looks much better.
Thanks, this looks much better.
> fs/erofs/inode.c| 35 ++-
> fs/erofs/internal.h | 10 --
> fs/erofs/super.c| 5 ++---
> 3 files changed, 12 insertions(+), 38 deletions(-)
And that diffstat ain't bad either.
> {
> - 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.
> + 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.
This looks much better now.
On Sun, Sep 01, 2019 at 01:51:13PM +0800, Gao Xiang wrote:
> 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
On Sun, Sep 01, 2019 at 01:51:14PM +0800, Gao Xiang wrote:
> 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
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.
On Sun, Sep 01, 2019 at 01:51:15PM +0800, Gao Xiang wrote:
> From: Gao Xiang
>
> As Christoph said [1] "having this function seems
> entirely pointless", let's kill those.
Looks much better.
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
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.
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 {
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
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;
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
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 <
On Mon, Sep 02, 2019 at 10:43:03AM +0200, Pavel Machek wrote:
> > > > 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
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
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
>
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,
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
>
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
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
> @@ -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,
On Sun, Sep 01, 2019 at 01:51:30PM +0800, Gao Xiang wrote:
> From: Gao Xiang
>
> As Christoph said [1], ".. and save one
> level of indentation."
Thanks. Just a little cleanup, but cumulated things like this really
help readability.
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
On 2019-9-2 21:06, David Sterba wrote:
> On Mon, Sep 02, 2019 at 05:57:11AM -0700, Christoph Hellwig wrote:
>>> +config EROFS_FS_XATTR
>>> + bool "EROFS extended attributes"
>>> + depends on EROFS_FS
>>> + default y
>>> + help
>>> + Extended attributes are name:value pairs associated
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,
> >
Thanks,
much better. I'm still hoping REQ_PRIO in this current form will go
entirely away soon as well.
On Sun, Sep 01, 2019 at 01:51:22PM +0800, Gao Xiang wrote:
> 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.
A lot
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
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,
>
> +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
> +
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
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
> > +
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;
> >
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
On Mon, Sep 02, 2019 at 09:51:59PM +0800, Chao Yu wrote:
> On 2019-9-2 21:06, David Sterba wrote:
> > On Mon, Sep 02, 2019 at 05:57:11AM -0700, Christoph Hellwig wrote:
> >>> +config EROFS_FS_XATTR
> >>> + bool "EROFS extended attributes"
> >>> + depends on EROFS_FS
> >>> + default y
> >>> + help
Thanks. I don't have a tree with all these applies, but please make
sure this covers at least all inlines in headers and all methods
wired up to operation structures.
Thanks,
this looks much nicer.
>
> - 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
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);
> >
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
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
> >
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);
> > > +
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
50 matches
Mail list logo
