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