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

[Jeff Layton <jlayton@kernel.org>]

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:-/
> 

The default is 4k for the userland client, and the kernel client had it
hardcoded at 4k (though it seemed to swap in PAGE_SIZE in random places
in the code).

I think the MDS allows the client to inline an arbitrary size of data
but there are probably some limits I don't see. I have no idea how the
client is intended to discover this value...

The whole inlining feature was somewhat half-baked, IMNSHO.

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

Probably. This code is on the way out of ceph and (eventually) the
kernel, so I'm not inclined to worry too much about subtle bugs in here.
-- 
Jeff Layton <jlayton@kernel.org>