Re: [PATCH] mm/filemap: do not allocate cache pages beyond end of file at read

From: Steven Whitehouse
Date: Thu Oct 31 2019 - 07:41:19 EST


On 30/10/2019 10:54, Linus Torvalds wrote:
On Wed, Oct 30, 2019 at 11:35 AM Steven Whitehouse<swhiteho@xxxxxxxxxx> wrote:
NFS may be ok here, but it will break GFS2. There may be others too...
OCFS2 is likely one. Not sure about CIFS either. Does it really matter
that we might occasionally allocate a page and then free it again?
Why are gfs2 and cifs doing things wrong?
For CIFS I've added Ronnie and Steve to common on that.
"readpage()" is not for synchrionizing metadata. Never has been. You
shouldn't treat it that way, and you shouldn't then make excuses for
filesystems that treat it that way.

Look at mmap, for example. It will do the SIGBUS handling before
calling readpage(). Same goes for the copyfile code. A filesystem that
thinks "I will update size at readpage" is already fundamentally

We do _recheck_ the inode size under the page lock, but that's to
handle the races with truncate etc.


For the GFS2 side of things, the algorithm looks like this:

Â- Is there an uptodate page in cache?

ÂÂ Yes, return it

ÂÂ No, call into the fs readpage() to get one

This is designed so that for pages that are available in the page cache, we don't even need to call into the filesystem at all. It is all dealt with at the page cache level, unless the page doesn't exist. At this point we don't know what the i_size might be, and prior to the proposed patch, it simply doesn't matter, since we will ask the filesystem via ->readpage() for all pages which are not in the cache.

If the page doesn't exist, we have to take the cluster level locks (glocks in the case of GFS2) which are potentially expensive, certainly a lot more expensive than the page lock anyway. That is currently done at the ->readpage() level, although we do have to drop the page lock first and then get the locks in the correct order, since the lock ordering requires the glock to be taken in shared mode ahead of the page lock.

We've always in the past been able to just use the generic code, since it was written to not assume i_size was valid outside of the fs specific locks. The aim has always been to try and use generic code as much as possible, even though there are some cases where we've had to depart from that for various reasons.

It appears that the filemap_fault issue seems to have not been spotted before. I'm not quite sure how that was missed - seems to show that we have some missing tests, but I agree that it does need to be fixed. It is a while since I last looked at that particular bit of code in detail, so my memory may be a bit fuzzy.

Andreas, Bob, have I missed anything here?