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