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

From: Jeff Layton <jlayton@kernel.org>
To: David Howells <dhowells@redhat.com>,
	Matthew Wilcox <willy@infradead.org>
Cc: ceph-devel@vger.kernel.org, linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH 2/3] ceph: Uninline the data on a file opened for writing
Date: Fri, 21 Jan 2022 09:03:14 -0500	[thread overview]
Message-ID: <35bb39af7b59920dc6160d69581cca987e8f4a36.camel@kernel.org> (raw)
In-Reply-To: <2883819.1642438775@warthog.procyon.org.uk>

On Mon, 2022-01-17 at 16:59 +0000, David Howells wrote:
> 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.
> 

It probably should have done uninlining on truncate(). OTOH, the old
code never did this, so I'm not inclined to add that in. 

I'd feel differently if we were looking to keep the inline_data as a
feature, long-term, but we aren't. The plan is to keep this feature
limping along in the kclient until the last ceph release that supports
inline_data is no longer supported in the field, at which point we'll
rip support out altogether.

We officially marked it deprecated in the Octopus release, and I was
hoping to remove it from ceph in Quincy (which should ship this
spring). We still need the cephfs-scrub utility to walk the fs and
uninline any files that are still inlined so we can support people on
upgrades. That work isn't finished yet. See:

    https://tracker.ceph.com/issues/52916

-- 
Jeff Layton <jlayton@kernel.org>