Re: [PATCH 2/3] ceph: Uninline the data on a file opened for writing

From: David Howells <dhowells@redhat.com>
To: Matthew Wilcox <willy@infradead.org>
Cc: dhowells@redhat.com, ceph-devel@vger.kernel.org,
	jlayton@kernel.org, linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH 2/3] ceph: Uninline the data on a file opened for writing
Date: Mon, 17 Jan 2022 16:59:35 +0000	[thread overview]
Message-ID: <2883819.1642438775@warthog.procyon.org.uk> (raw)
In-Reply-To: <YeWdlR7nsBG8fYO2@casper.infradead.org>

Matthew Wilcox <willy@infradead.org> wrote:

> > +	if (folio_test_uptodate(folio))
> > +		goto out_put_folio;
> 
> Er ... if (!folio_test_uptodate(folio)), perhaps?  And is it even
> worth testing if read_mapping_folio() returned success?  I feel like
> we should take ->readpage()'s word for it that success means the
> folio is now uptodate.

Actually, no, I shouldn't need to do this since it comes out with the page
lock still held.

> > +	len = i_size_read(inode);
> > +	if (len >  folio_size(folio))
> 
> extra space.  Plus, you're hardcoding 4096 below, but using folio_size()
> here which is a bit weird to me.

As I understand it, 4096 is the maximum length of the inline data, not
PAGE_SIZE, so I have to be careful when doing a DIO read because it might
start after the data - and there's also truncate to consider:-/

I wonder if the uninlining code should lock the inode while it does it and the
truncation code should do uninlining too.

David