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:47 +0000, Matthew Wilcox wrote:
> On Mon, Jan 17, 2022 at 04:26:36PM +0000, David Howells wrote:
> > +	folio = read_mapping_folio(inode->i_mapping, 0, file);
> > +	if (IS_ERR(folio))
> > +		goto out;
> 
> ... you need to set 'err' here, right?
> 
> > +	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.
> 
> > +	err = folio_lock_killable(folio);
> > +	if (err < 0)
> > +		goto out_put_folio;
> > +
> > +	if (inline_version == 1 || /* initial version, no data */
> > +	    inline_version == CEPH_INLINE_NONE)
> > +		goto out_unlock;
> > +
> > +	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.

The client actually decides how big a region to inline when it does
this. The default is 4k, but someone could inline up to 64k (and
potentially larger, if they tweak settings the right way).

I'd suggest not capping the length in this function at all. If you find
more than 4k, just assume that some other client stashed more data than
expected, and uninline whatever is there.

You might also consider throwing in a pr_warn or something in that case,
since finding more than 4k uninlined would be unexpected (though not
necessarily broken).
-- 
Jeff Layton <jlayton@kernel.org>