linux-kernel - Re: [PATCH v14 102/138] iomap: Convert iomap_write_begin and iomap_write

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YPD476gr7MWsaAyr@casper.infradead.org>
Date:   Fri, 16 Jul 2021 04:11:43 +0100
From:   Matthew Wilcox <willy@...radead.org>
To:     "Darrick J. Wong" <djwong@...nel.org>
Cc:     linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH v14 102/138] iomap: Convert iomap_write_begin and
 iomap_write_end to folios

On Thu, Jul 15, 2021 at 02:51:05PM -0700, Darrick J. Wong wrote:
> On Thu, Jul 15, 2021 at 04:36:28AM +0100, Matthew Wilcox (Oracle) wrote:
> > +static int iomap_write_begin(struct inode *inode, loff_t pos, size_t len,
> > +		unsigned flags, struct folio **foliop, struct iomap *iomap,
> > +		struct iomap *srcmap)
> >  {
> >  	const struct iomap_page_ops *page_ops = iomap->page_ops;
> > +	struct folio *folio;
> >  	struct page *page;
> > +	unsigned fgp = FGP_LOCK | FGP_WRITE | FGP_CREAT | FGP_STABLE | FGP_NOFS;
> >  	int status = 0;
> >  
> >  	BUG_ON(pos + len > iomap->offset + iomap->length);
> > @@ -604,30 +605,31 @@ iomap_write_begin(struct inode *inode, loff_t pos, unsigned len, unsigned flags,
> >  			return status;
> >  	}
> >  
> > -	page = grab_cache_page_write_begin(inode->i_mapping, pos >> PAGE_SHIFT,
> > -			AOP_FLAG_NOFS);
> > -	if (!page) {
> > +	folio = __filemap_get_folio(inode->i_mapping, pos >> PAGE_SHIFT, fgp,
> 
> Ah, ok, so we're moving the file_get_pages flags up to iomap now.

Right, saves us having a folio equivalent of
grab_cache_page_write_begin().  And lets us get rid of AOP_FLAG_NOFS
eventually (although that really should be obsoleted by scoped
allocations, but one windmill at a time).

> > +	struct page *page = folio_file_page(folio, pos / PAGE_SIZE);
> 
> pos >> PAGE_SHIFT ?

mmm.  We're inconsistent:

willy@...e:~/kernel/folio$ git grep '/ PAGE_SIZE' mm/ fs/ |wc
     92     720    6475
willy@...e:~/kernel/folio$ git grep '>> PAGE_SHIFT' mm/ fs/ |wc
    635    4582   39394

That said, there's a clear preference.  It's just that we had a bug the
other day where somebody shifted by PAGE_SHIFT in the wrong direction ...
But again, this is your code, so I'll change to the shift.