Re: [PATCH V10 03/19] block: use bio_for_each_bvec() to compute multi-page bvec count

On Thu, Nov 15, 2018 at 02:18:47PM -0800, Omar Sandoval wrote: > My only reason to prefer unsigned int is consistency. unsigned int is > much more common in the kernel: > > $ ag --cc -s 'unsigned\s+int' | wc -l > 129632 > $ ag --cc -s 'unsigned\s+(?!char|short|int|long)' | wc -l > 22435 > > check

Re: [PATCH V10 01/19] block: introduce multi-page page bvec helpers

> -#define bvec_iter_page(bvec, iter) \ > +#define mp_bvec_iter_page(bvec, iter)\ > (__bvec_iter_bvec((bvec), (iter))->bv_page) > > -#define bvec_iter_len(bvec, iter)\ > +#define mp_bvec_iter_len(bvec, ite

Re: [PATCH V10 02/19] block: introduce bio_for_each_bvec()

> +static inline void __bio_advance_iter(struct bio *bio, struct bvec_iter > *iter, > + unsigned bytes, bool mp) I think these magic 'bool np' arguments and wrappers over wrapper don't help anyone to actually understand the code. I'd vote for removing as many wr

Re: [PATCH V10 04/19] block: use bio_for_each_bvec() to map sg

> + if (!*sg) > + return sglist; > + else { No need for an else after an early return.

Re: [PATCH V10 05/19] block: introduce bvec_last_segment()

> + unsigned last_page = total / PAGE_SIZE; > + > + if (last_page * PAGE_SIZE == total) Not sure it matters much with modern compilers, but generally we shit by PAGE_SHIFT instead of multiplying with or dividing by PAGE_SIZE.

Re: [PATCH V10 06/19] fs/buffer.c: use bvec iterator to truncate the bio

Looks fine, Reviewed-by: Christoph Hellwig

Re: [PATCH V10 07/19] btrfs: use bvec_last_segment to get bio's last page

On Thu, Nov 15, 2018 at 04:52:54PM +0800, Ming Lei wrote: > index 2955a4ea2fa8..161e14b8b180 100644 > --- a/fs/btrfs/compression.c > +++ b/fs/btrfs/compression.c > @@ -400,8 +400,11 @@ blk_status_t btrfs_submit_compressed_write(struct inode > *inode, u64 start, > static u64 bio_end_offset(struct

Re: [PATCH V10 08/19] btrfs: move bio_pages_all() to btrfs

On Thu, Nov 15, 2018 at 04:52:55PM +0800, Ming Lei wrote: > BTRFS is the only user of this helper, so move this helper into > BTRFS, and implement it via bio_for_each_segment_all(), since > bio->bi_vcnt may not equal to number of pages after multipage bvec > is enabled. btrfs only uses the value t

Re: [PATCH V10 09/19] block: introduce bio_bvecs()

On Thu, Nov 15, 2018 at 04:52:56PM +0800, Ming Lei wrote: > There are still cases in which we need to use bio_bvecs() for get the > number of multi-page segment, so introduce it. The only user in your final tree seems to be the loop driver, and even that one only uses the helper for read/write bio

Re: [PATCH V10 11/19] bcache: avoid to use bio_for_each_segment_all() in bch_bio_alloc_pages()

> - bio_for_each_segment_all(bv, bio, i) { > + for (i = 0, bv = bio->bi_io_vec; i < bio->bi_vcnt; bv++) { This really needs a comment. Otherwise it looks fine to me.

Re: [PATCH V10 13/19] iomap & xfs: only account for new added page

I'd much rather have __bio_try_merge_page only do merges in the same page, and have a new __bio_try_merge_segment that does multi-page merges. This will keep the accounting a lot simpler.

Re: [PATCH V10 14/19] block: enable multipage bvecs

> - > - if (page == bv->bv_page && off == bv->bv_offset + bv->bv_len) { > - bv->bv_len += len; > - bio->bi_iter.bi_size += len; > - return true; > - } > + struct request_queue *q = NULL; > + > +

Re: [PATCH V10 15/19] block: always define BIO_MAX_PAGES as 256

> 256 > -#define BIO_MAX_PAGESHPAGE_PMD_NR > -#else > #define BIO_MAX_PAGES256 > -#endif > -#else > -#define BIO_MAX_PAGES256 > -#endif Looks good. This mess should have never gone in. Reviewed-by: Christoph Hellwig

Re: [PATCH V10 17/19] block: don't use bio->bi_vcnt to figure out segment number

good, but shouldn't this go to the beginning of the series? Reviewed-by: Christoph Hellwig

Re: [PATCH V10 18/19] block: kill QUEUE_FLAG_NO_SG_MERGE

> Cc: linux-bt...@vger.kernel.org > Cc: Darrick J. Wong > Cc: linux-...@vger.kernel.org > Cc: Gao Xiang > Cc: Christoph Hellwig > Cc: Theodore Ts'o > Cc: linux-e...@vger.kernel.org > Cc: Coly Li > Cc: linux-bca...@vger.kernel.org > Cc: Boaz Harrosh > Cc: Bob

Re: [PATCH V10 18/19] block: kill QUEUE_FLAG_NO_SG_MERGE

On Thu, Nov 15, 2018 at 06:18:11PM -0800, Omar Sandoval wrote: > This commit message wasn't very clear. Is it the case that > QUEUE_FLAG_NO_SG_MERGE is no longer set by any drivers? I think he wants to say that not doing S/G merging is rather pointless with the current setup of the I/O path, as it

Re: [PATCH V10 19/19] block: kill BLK_MQ_F_SG_MERGE

On Thu, Nov 15, 2018 at 04:53:06PM +0800, Ming Lei wrote: > QUEUE_FLAG_NO_SG_MERGE has been killed, so kill BLK_MQ_F_SG_MERGE too. Looks fine, Reviewed-by: Christoph Hellwig

Re: [PATCH V10 00/19] block: support multi-page bvec

It seems like bi_phys_segments is still around of this series. Shouldn't it be superflous now?

Re: [PATCH V10 08/19] btrfs: move bio_pages_all() to btrfs

On Mon, Nov 19, 2018 at 04:19:24PM +0800, Ming Lei wrote: > On Fri, Nov 16, 2018 at 02:38:45PM +0100, Christoph Hellwig wrote: > > On Thu, Nov 15, 2018 at 04:52:55PM +0800, Ming Lei wrote: > > > BTRFS is the only user of this helper, so move this helper into > > >

Re: [PATCH V10 09/19] block: introduce bio_bvecs()

On Mon, Nov 19, 2018 at 04:49:27PM -0800, Sagi Grimberg wrote: > >> The only user in your final tree seems to be the loop driver, and >> even that one only uses the helper for read/write bios. >> >> I think something like this would be much simpler in the end: > > The recently submitted nvme-tcp ho

Re: [PATCH V10 09/19] block: introduce bio_bvecs()

On Tue, Nov 20, 2018 at 09:35:07PM -0800, Sagi Grimberg wrote: >> Given it is over TCP, I guess it should be doable for you to preallocate one >> 256-bvec table in one page for each request, then sets the max segment size >> as >> (unsigned int)-1, and max segment number as 256, the preallocated t

Re: Some new bio merging behaviors in __bio_try_merge_page

On Thu, Apr 11, 2019 at 04:09:54PM +0800, Ming Lei wrote: > I don't think it is a good behaviour to use bio->bi_max_vecs to limit > max allowed page, you may see the idea from the naming simply... > > If there were other such drivers, we may fix it easily, and the following > patch should fix your

Re: [PATCH] staging: erofs: fix unexpected out-of-bound data access

> +++ b/drivers/staging/erofs/data.c > @@ -304,7 +304,7 @@ static inline struct bio *erofs_read_raw_page(struct bio > *bio, > *last_block = current_block; > > /* shift in advance in case of it followed by too many gaps */ > - if (unlikely(bio->bi_vcnt >= bio->bi_max_vecs)) { > +

Re: [PATCH] staging: erofs: fix unexpected out-of-bound data access

On Fri, Apr 12, 2019 at 08:06:33AM -0700, Christoph Hellwig wrote: > > +++ b/drivers/staging/erofs/data.c > > @@ -304,7 +304,7 @@ static inline struct bio *erofs_read_raw_page(struct > > bio *bio, > > *last_block = current_block; > > > > /* shift in a

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

On Sun, Aug 18, 2019 at 11:11:54AM -0400, Theodore Y. Ts'o wrote: > Note that of the mainstream file systems, ext4 and xfs don't guarantee > that it's safe to blindly take maliciously provided file systems, such > as those provided by a untrusted container, and mount it on a file > system without p

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

On Sun, Aug 18, 2019 at 09:16:38AM -0700, Eric Biggers wrote: > Ted's observation was about maliciously-crafted filesystems, though, so > integrity-only features such as metadata checksums are irrelevant. Also the > filesystem version is irrelevant; anything accepted by the kernel code (even > if

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

On Sun, Aug 18, 2019 at 10:29:38AM -0700, Eric Biggers wrote: > Not sure what you're even disagreeing with, as I *do* expect new filesystems > to > be held to a high standard, and to be written with the assumption that the > on-disk data may be corrupted or malicious. We just can't expect the bar

Re: [PATCH v8] Add flags option to get xattr method paired to __vfs_getxattr

On Tue, Aug 27, 2019 at 08:05:15AM -0700, Mark Salyzyn wrote: > Replace arguments for get and set xattr methods, and __vfs_getxattr > and __vfs_setaxtr functions with a reference to the following now > common argument structure: Yikes. That looks like a mess. Why can't we pass a kernel-only flag

Re: [PATCH v6 01/24] erofs: add on-disk layout

> --- /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. > +struct erofs_super_block { > +/* 0 */__le32 magic; /* in

Re: [PATCH v6 03/24] erofs: add super block operations

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, > +

Re: [PATCH v6 04/24] erofs: add raw address_space operations

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.

Re: [PATCH v6 05/24] erofs: add inode operations

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

Re: [PATCH v6 06/24] erofs: support special inode

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(-) > > di

Re: [PATCH v6 08/24] erofs: add namei functions

On Fri, Aug 02, 2019 at 08:53:31PM +0800, Gao Xiang wrote: > +struct erofs_qstr { > + const unsigned char *name; > + const unsigned char *end; > +}; Maybe erofs_name? The q in qstr stands for quick, because of the existing hash and len, which this doesn't really provide. Also I don't rea

Re: [PATCH v6 01/24] erofs: add on-disk layout

On Thu, Aug 29, 2019 at 06:32:53PM +0800, Gao Xiang wrote: > I can fix it up as you like but I still cannot get > what is critical issues here. The problem is that the whole codebase is way substandard quality, looking a lot like Linux code from 20 years ago. Yes, we already have plenty of code o

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

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; >

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

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 a

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

> - 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 imp

Re: [PATCH v8 20/24] erofs: introduce generic decompression backend

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?

Re: [PATCH v6 03/24] erofs: add super block operations

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 f

Re: [PATCH v6 04/24] erofs: add raw address_space operations

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 do

Re: [PATCH v6 05/24] erofs: add inode operations

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_d

Re: [PATCH v8 20/24] erofs: introduce generic decompression backend

On Sat, Aug 31, 2019 at 12:52:17AM +0800, Gao Xiang wrote: > 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; > > > +

Re: [PATCH 01/21] erofs: remove all the byte offset comments

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 poi

Re: [PATCH 02/21] erofs: on-disk format should have explicitly assigned numbers

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, > + EROFS_INODE_FLAT_COMP

Re: [PATCH 03/21] erofs: some macros are much more readable as a function

This looks much better now.

Re: [PATCH 04/21] erofs: kill __packed for on-disk structures

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

Re: [PATCH 05/21] erofs: update erofs_inode_is_data_compressed helper

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 ha

Re: [PATCH 06/21] erofs: kill erofs_{init,exit}_inode_cache

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.

Re: [PATCH 07/21] erofs: use erofs_inode naming

> { > - struct erofs_vnode *vi = ptr; > - > - inode_init_once(&vi->vfs_inode); > + inode_init_once(&((struct erofs_inode *)ptr)->vfs_inode); Why doesn't this use EROFS_I? This looks a little odd.

Re: [PATCH 10/21] erofs: kill is_inode_layout_compression()

Thanks, this looks much better.

Re: [PATCH 09/21] erofs: update erofs symlink stuffs

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.

Re: [PATCH 11/21] erofs: use dsb instead of layout for ondisk super_block

> + 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.

Re: [PATCH 12/21] erofs: kill verbose debug info in erofs_fill_super

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

Re: [PATCH 13/21] erofs: simplify erofs_grab_bio() since bio_alloc() never fail

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 of

Re: [PATCH 14/21] erofs: kill prio and nofail of erofs_get_meta_page()

Thanks, much better. I'm still hoping REQ_PRIO in this current form will go entirely away soon as well.

Re: [PATCH 16/21] erofs: kill magic underscores

> > - 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

Re: [PATCH 18/21] erofs: add "erofs_" prefix for common and short functions

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.

Re: [PATCH 17/21] erofs: use a switch statement when dealing with the file modes

Thanks, this looks much nicer.

Re: [PATCH 19/21] erofs: kill all erofs specific fault injection

On Sun, Sep 01, 2019 at 01:51:28PM +0800, Gao Xiang wrote: > 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." Thanks.

Re: [PATCH 20/21] erofs: kill use_vmap module parameter

> @@ -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);

Re: [PATCH 21/21] erofs: save one level of indentation

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.

Re: [PATCH v6 01/24] erofs: add on-disk layout

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_L

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

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, >

Re: [PATCH 07/21] erofs: use erofs_inode naming

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(&vi->vfs_inode);

Re: [PATCH v6 03/24] erofs: add super block operations

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

Re: [PATCH v6 05/24] erofs: add inode operations

On Sun, Sep 01, 2019 at 05:34:00PM +0800, Gao Xiang wrote: > > > + return iget5_locked(sb, hashval, erofs_ilookup_test_actor, > > > + erofs_iget_set_actor, &nid); > > > +#endif > > > > Just use the slightly more complicated 32-bit version everywhere so that > > you have a single actually t

Re: [PATCH 16/21] erofs: kill magic underscores

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

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

> +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 > +

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

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 b

Re: [PATCH v6 03/24] erofs: add super block operations

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

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

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

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

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

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

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

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

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, >

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

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.

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. And 4 out of 6 also define EFSBADCRC, so please lift that as well.

Re: [PATCH v5 02/13] mm: Ignore return value of ->readpages

On Mon, Feb 10, 2020 at 05:03:37PM -0800, Matthew Wilcox wrote: > From: "Matthew Wilcox (Oracle)" > > We used to assign the return value to a variable, which we then ignored. > Remove the pretence of caring. > > Signed-off-by: Matthew Wilcox (Oracle) Looks g

Re: [PATCH v5 01/13] mm: Fix the return type of __do_page_cache_readahead

Looks good, Reviewed-by: Christoph Hellwig

Re: [PATCH v5 04/13] mm: Add readahead address space operation

On Mon, Feb 10, 2020 at 05:03:39PM -0800, Matthew Wilcox wrote: > +struct readahead_control { > + struct file *file; > + struct address_space *mapping; > +/* private: use the readahead_* accessors instead */ > + pgoff_t start; > + unsigned int nr_pages; > + unsigned int batch_co

Re: [PATCH v6 07/19] mm: Put readahead pages in cache earlier

On Wed, Feb 19, 2020 at 06:41:17AM -0800, Matthew Wilcox wrote: > #define readahead_for_each(rac, page) \ > while ((page = readahead_page(rac))) > > No more readahead_next() to forget to add to filesystems which don't use > the readahead_for_each() iterato

Re: [PATCH v7 14/24] btrfs: Convert from readpages to readahead

On Thu, Feb 20, 2020 at 05:48:49AM -0800, Matthew Wilcox wrote: > btrfs: Convert from readpages to readahead > > Implement the new readahead method in btrfs. Add a readahead_page_batch() > to optimise fetching a batch of pages at once. Shouldn't this readahead_page_batch heper go into a separa

Re: [PATCH v7 21/24] iomap: Restructure iomap_readpages_actor

On Wed, Feb 19, 2020 at 01:01:00PM -0800, Matthew Wilcox wrote: > From: "Matthew Wilcox (Oracle)" > > By putting the 'have we reached the end of the page' condition at the end > of the loop instead of the beginning, we can remove the 'submit the last > page' code from iomap_readpages(). Also che

Re: [PATCH v7 22/24] iomap: Convert from readpages to readahead

On Wed, Feb 19, 2020 at 01:01:01PM -0800, Matthew Wilcox wrote: > From: "Matthew Wilcox (Oracle)" > > Use the new readahead operation in iomap. Convert XFS and ZoneFS to > use it. > > Signed-off-by: Matthew Wilcox (Oracle) > --- > fs/iomap/buffered-io.c | 90 +++---

Re: [PATCH v7 14/24] btrfs: Convert from readpages to readahead

On Thu, Feb 20, 2020 at 07:54:52AM -0800, Matthew Wilcox wrote: > On Thu, Feb 20, 2020 at 07:46:58AM -0800, Christoph Hellwig wrote: > > On Thu, Feb 20, 2020 at 05:48:49AM -0800, Matthew Wilcox wrote: > > > btrfs: Convert from readpages to readahead > > > > &

Re: [PATCH v7 01/24] mm: Move readahead prototypes from mm.h

al.h instead. Remove the parameter names where > they add no value, and rename the ones which were actively misleading. Looks good, Reviewed-by: Christoph Hellwig

Re: [PATCH v7 04/24] mm: Move readahead nr_pages check into read_pages

On Wed, Feb 19, 2020 at 01:00:43PM -0800, Matthew Wilcox wrote: > From: "Matthew Wilcox (Oracle)" > > Simplify the callers by moving the check for nr_pages and the BUG_ON > into read_pages(). > > Signed-off-by: Matthew Wilcox (Oracle) Looks good, Reviewed-by: Christoph Hellwig

Re: [PATCH v7 02/24] mm: Return void from various readahead functions

Looks good, Reviewed-by: Christoph Hellwig

Re: [PATCH v7 05/24] mm: Use readahead_control to pass arguments

no real need to initialize fields to zero if we initialize the structure at all. And while I'd normally not care that much, having as few references as possible to the _-prefixed internal variables helps making clear how internal they are. Otherwise looks good: Reviewed-by: Christoph Hellwig

Re: [PATCH v7 08/24] mm: Remove 'page_offset' from readahead loop

On Wed, Feb 19, 2020 at 01:00:47PM -0800, Matthew Wilcox wrote: > From: "Matthew Wilcox (Oracle)" > > Replace the page_offset variable with 'index + i'. > > Signed-off-by: Matthew Wilcox (Oracle) Looks good, Reviewed-by: Christoph Hellwig

Re: [PATCH v7 09/24] mm: Put readahead pages in cache earlier

gt; > /* > @@ -165,9 +164,11 @@ void __do_page_cache_readahead(struct address_space > *mapping, > LIST_HEAD(page_pool); > loff_t isize = i_size_read(inode); > gfp_t gfp_mask = readahead_gfp_mask(mapping); > + bool use_list = mapping->a_ops->readpages; I find this single use variable a little weird. Not a dealbreaker, but just checking the methods would seem a little more obvious to me. Except for this and the other nitpick the patch looks good to me: Reviewed-by: Christoph Hellwig

Re: [PATCH v7 10/24] mm: Add readahead address space operation

Looks fine modulo the little data type issue: Reviewed-by: Christoph Hellwig

Re: [PATCH v7 14/24] btrfs: Convert from readpages to readahead

On Thu, Feb 20, 2020 at 07:57:27AM -0800, Christoph Hellwig wrote: > On Thu, Feb 20, 2020 at 07:54:52AM -0800, Matthew Wilcox wrote: > > On Thu, Feb 20, 2020 at 07:46:58AM -0800, Christoph Hellwig wrote: > > > On Thu, Feb 20, 2020 at 05:48:49AM -0800, Matthew Wilcox wrote: >

Re: [Cluster-devel] [PATCH v7 12/24] mm: Add page_cache_readahead_unbounded

t directly. I don't like this, but then I like the horrible open coded versions even less.. Can you add a do not use for new code comment to the function as well? Otherwise looks good: Reviewed-by: Christoph Hellwig

Re: [Cluster-devel] [PATCH v7 13/24] fs: Convert mpage_readpages to mpage_readahead

allers are all trivial except for GFS2 & OCFS2. Looks sensible: Reviewed-by: Christoph Hellwig

Re: [PATCH v7 14/24] btrfs: Convert from readpages to readahead

On Mon, Feb 24, 2020 at 01:54:14PM -0800, Matthew Wilcox wrote: > > First I think the implicit ARRAY_SIZE in readahead_page_batch is highly > > dangerous, as it will do the wrong thing when passing a pointer or > > function argument. > > somebody already thought of that ;-) > > #define ARRAY_SIZE

Re: [PATCH v7 21/24] iomap: Restructure iomap_readpages_actor

On Thu, Feb 20, 2020 at 08:24:04AM -0800, Matthew Wilcox wrote: > On Thu, Feb 20, 2020 at 07:47:41AM -0800, Christoph Hellwig wrote: > > On Wed, Feb 19, 2020 at 01:01:00PM -0800, Matthew Wilcox wrote: > > > From: "Matthew Wilcox (Oracle)" > > > > > &

Re: [PATCH v8 05/25] mm: Add new readahead_control API

arge pages, even though none of the filesystems > to be converted do yet. > > Signed-off-by: Matthew Wilcox (Oracle) Looks good, Reviewed-by: Christoph Hellwig

Re: [PATCH v8 14/25] mm: Document why we don't set PageReadahead

On Tue, Feb 25, 2020 at 01:48:27PM -0800, Matthew Wilcox wrote: > From: "Matthew Wilcox (Oracle)" > > If the page is already in cache, we don't set PageReadahead on it. > > Signed-off-by: Matthew Wilcox (Oracle) Looks good, Reviewed-by: Christoph Hellwig

Re: [PATCH v8 17/25] btrfs: Convert from readpages to readahead

On Tue, Feb 25, 2020 at 01:48:30PM -0800, Matthew Wilcox wrote: > From: "Matthew Wilcox (Oracle)" > > Implement the new readahead method in btrfs. Add a readahead_page_batch() > to optimise fetching a batch of pages at once. readahead_page_batch() isn't added in this patch anymore. Otherwise t

  1   2   3   4   >