[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240617095837.bzf4xiv2jxv6j7vt@quentin>
Date: Mon, 17 Jun 2024 09:58:37 +0000
From: "Pankaj Raghav (Samsung)" <kernel@...kajraghav.com>
To: Christoph Hellwig <hch@....de>
Cc: david@...morbit.com, djwong@...nel.org, chandan.babu@...cle.com,
brauner@...nel.org, akpm@...ux-foundation.org, willy@...radead.org,
mcgrof@...nel.org, linux-mm@...ck.org, hare@...e.de,
linux-kernel@...r.kernel.org, yang@...amperecomputing.com,
Zi Yan <zi.yan@...t.com>, linux-xfs@...r.kernel.org,
p.raghav@...sung.com, linux-fsdevel@...r.kernel.org,
gost.dev@...sung.com, cl@...amperecomputing.com,
john.g.garry@...cle.com
Subject: Re: [PATCH v7 03/11] filemap: allocate mapping_min_order folios in
the page cache
On Thu, Jun 13, 2024 at 10:44:10AM +0200, Christoph Hellwig wrote:
> On Fri, Jun 07, 2024 at 02:58:54PM +0000, Pankaj Raghav (Samsung) wrote:
> > +static inline unsigned long mapping_min_folio_nrpages(struct address_space *mapping)
> > +{
> > + return 1UL << mapping_min_folio_order(mapping);
> > +}
>
> Overly long line here, just line break after the return type.
>
> Then again it only has a single user just below and no documentation
> so maybe just fold it into the caller?
I do use it in later patches. I will adjust the long line here :)
>
> > no_page:
> > if (!folio && (fgp_flags & FGP_CREAT)) {
> > - unsigned order = FGF_GET_ORDER(fgp_flags);
> > + unsigned int min_order = mapping_min_folio_order(mapping);
> > + unsigned int order = max(min_order, FGF_GET_ORDER(fgp_flags));
> > int err;
> > + index = mapping_align_start_index(mapping, index);
>
> I wonder if at some point splitting this block that actually allocates
> a new folio into a separate helper would be nice. It just keep growing
> in size and complexity.
>
I agree with that. I will put it in my future todo backlog.
> > - folio = filemap_alloc_folio(mapping_gfp_mask(mapping), 0);
> > + folio = filemap_alloc_folio(mapping_gfp_mask(mapping),
> > + min_order);
>
> Nit: no need to split this into multiple lines.
Ok.
>
> > if (!folio)
> > return -ENOMEM;
> >
> > @@ -2471,6 +2478,8 @@ static int filemap_create_folio(struct file *file,
> > * well to keep locking rules simple.
> > */
> > filemap_invalidate_lock_shared(mapping);
> > + /* index in PAGE units but aligned to min_order number of pages. */
>
> in PAGE_SIZE units? Maybe also make this a complete sentence?
Yes, will do.
>
Powered by blists - more mailing lists