On Mon, 2022-01-17 at 16:59 +0000, David Howells wrote: > Matthew Wilcox <willy@xxxxxxxxxxxxx> 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@xxxxxxxxxx>
