| Message ID | 20230602222445.2284892-3-willy@infradead.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Create large folios in iomap buffered write path | expand |
On Fri, Jun 02, 2023 at 11:24:39PM +0100, Matthew Wilcox (Oracle) wrote: > The filesystem ->release_folio method is called under more circumstances > now than when the documentation was written. The second sentence > describing the interpretation of the return value is the wrong polarity > (false indicates failure, not success). And the third sentence is also > wrong (the kernel calls try_to_free_buffers() instead). > > So replace the entire paragraph with a detailed description of what the > state of the folio may be, the meaning of the gfp parameter, why the > method is being called and what the filesystem is expected to do. > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > --- > Documentation/filesystems/locking.rst | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/Documentation/filesystems/locking.rst b/Documentation/filesystems/locking.rst > index aa1a233b0fa8..91dc9d5bc602 100644 > --- a/Documentation/filesystems/locking.rst > +++ b/Documentation/filesystems/locking.rst > @@ -374,10 +374,16 @@ invalidate_lock before invalidating page cache in truncate / hole punch > path (and thus calling into ->invalidate_folio) to block races between page > cache invalidation and page cache filling functions (fault, read, ...). > > -->release_folio() is called when the kernel is about to try to drop the > -buffers from the folio in preparation for freeing it. It returns false to > -indicate that the buffers are (or may be) freeable. If ->release_folio is > -NULL, the kernel assumes that the fs has no private interest in the buffers. > +->release_folio() is called when the MM wants to make a change to the > +folio that would invalidate the filesystem's private data. For example, > +it may be about to be removed from the address_space or split. The folio > +is locked and not under writeback. It may be dirty. The gfp parameter is > +not usually used for allocation, but rather to indicate what the filesystem > +may do to attempt to free the private data. The filesystem may > +return false to indicate that the folio's private data cannot be freed. > +If it returns true, it should have already removed the private data from > +the folio. If a filesystem does not provide a ->release_folio method, > +the kernel will call try_to_free_buffers(). the MM? Since you changed that above... :) With that nit fixed, Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > > ->free_folio() is called when the kernel has dropped the folio > from the page cache. > -- > 2.39.2 >
On Sun, Jun 04, 2023 at 10:55:48AM -0700, Darrick J. Wong wrote: > On Fri, Jun 02, 2023 at 11:24:39PM +0100, Matthew Wilcox (Oracle) wrote: > > -->release_folio() is called when the kernel is about to try to drop the > > -buffers from the folio in preparation for freeing it. It returns false to > > -indicate that the buffers are (or may be) freeable. If ->release_folio is > > -NULL, the kernel assumes that the fs has no private interest in the buffers. > > +->release_folio() is called when the MM wants to make a change to the > > +folio that would invalidate the filesystem's private data. For example, > > +it may be about to be removed from the address_space or split. The folio > > +is locked and not under writeback. It may be dirty. The gfp parameter is > > +not usually used for allocation, but rather to indicate what the filesystem > > +may do to attempt to free the private data. The filesystem may > > +return false to indicate that the folio's private data cannot be freed. > > +If it returns true, it should have already removed the private data from > > +the folio. If a filesystem does not provide a ->release_folio method, > > +the kernel will call try_to_free_buffers(). > > the MM? Since you changed that above... :) > > With that nit fixed, > Reviewed-by: Darrick J. Wong <djwong@kernel.org> Well, is it the MM? At this point, the decision is made by filemap_release_folio(), which is the VFS, in my opinion ;-) But I'm happy to use "the MM". Or "the pagecache".
On Sun, Jun 04, 2023 at 09:10:55PM +0100, Matthew Wilcox wrote: > On Sun, Jun 04, 2023 at 10:55:48AM -0700, Darrick J. Wong wrote: > > On Fri, Jun 02, 2023 at 11:24:39PM +0100, Matthew Wilcox (Oracle) wrote: > > > -->release_folio() is called when the kernel is about to try to drop the > > > -buffers from the folio in preparation for freeing it. It returns false to > > > -indicate that the buffers are (or may be) freeable. If ->release_folio is > > > -NULL, the kernel assumes that the fs has no private interest in the buffers. > > > +->release_folio() is called when the MM wants to make a change to the > > > +folio that would invalidate the filesystem's private data. For example, > > > +it may be about to be removed from the address_space or split. The folio > > > +is locked and not under writeback. It may be dirty. The gfp parameter is > > > +not usually used for allocation, but rather to indicate what the filesystem > > > +may do to attempt to free the private data. The filesystem may > > > +return false to indicate that the folio's private data cannot be freed. > > > +If it returns true, it should have already removed the private data from > > > +the folio. If a filesystem does not provide a ->release_folio method, > > > +the kernel will call try_to_free_buffers(). > > > > the MM? Since you changed that above... :) > > > > With that nit fixed, > > Reviewed-by: Darrick J. Wong <djwong@kernel.org> > > Well, is it the MM? At this point, the decision is made by > filemap_release_folio(), which is the VFS, in my opinion ;-) It's in mm/filemap.c, which I think makes it squarely the pagecache/mm, not the vfs. "Yuh better stop your train, rabbit!" etc ;) > But I'm happy to use "the MM". Or "the pagecache". Sounds good to me. --D
Never mind the shism on the nature of the holy trinity of
vfs/mm/pagecache the substance looks good here:
Reviewed-by: Christoph Hellwig <hch@lst.de>
On Sun, Jun 04, 2023 at 01:33:06PM -0700, Darrick J. Wong wrote: > On Sun, Jun 04, 2023 at 09:10:55PM +0100, Matthew Wilcox wrote: > > On Sun, Jun 04, 2023 at 10:55:48AM -0700, Darrick J. Wong wrote: > > > On Fri, Jun 02, 2023 at 11:24:39PM +0100, Matthew Wilcox (Oracle) wrote: > > > > -->release_folio() is called when the kernel is about to try to drop the > > > > -buffers from the folio in preparation for freeing it. It returns false to > > > > -indicate that the buffers are (or may be) freeable. If ->release_folio is > > > > -NULL, the kernel assumes that the fs has no private interest in the buffers. > > > > +->release_folio() is called when the MM wants to make a change to the > > > > +folio that would invalidate the filesystem's private data. For example, > > > > +it may be about to be removed from the address_space or split. The folio > > > > +is locked and not under writeback. It may be dirty. The gfp parameter is > > > > +not usually used for allocation, but rather to indicate what the filesystem > > > > +may do to attempt to free the private data. The filesystem may > > > > +return false to indicate that the folio's private data cannot be freed. > > > > +If it returns true, it should have already removed the private data from > > > > +the folio. If a filesystem does not provide a ->release_folio method, > > > > +the kernel will call try_to_free_buffers(). > > > > > > the MM? Since you changed that above... :) > > > > > > With that nit fixed, > > > Reviewed-by: Darrick J. Wong <djwong@kernel.org> > > > > Well, is it the MM? At this point, the decision is made by > > filemap_release_folio(), which is the VFS, in my opinion ;-) > > It's in mm/filemap.c, which I think makes it squarely the pagecache/mm, > not the vfs. Changed this to: If a filesystem does not provide a ->release_folio method, the pagecache will assume that private data is buffer_heads and call try_to_free_buffers().
On Mon, Jun 05, 2023 at 02:11:39PM +0100, Matthew Wilcox wrote: > On Sun, Jun 04, 2023 at 01:33:06PM -0700, Darrick J. Wong wrote: > > On Sun, Jun 04, 2023 at 09:10:55PM +0100, Matthew Wilcox wrote: > > > On Sun, Jun 04, 2023 at 10:55:48AM -0700, Darrick J. Wong wrote: > > > > On Fri, Jun 02, 2023 at 11:24:39PM +0100, Matthew Wilcox (Oracle) wrote: > > > > > -->release_folio() is called when the kernel is about to try to drop the > > > > > -buffers from the folio in preparation for freeing it. It returns false to > > > > > -indicate that the buffers are (or may be) freeable. If ->release_folio is > > > > > -NULL, the kernel assumes that the fs has no private interest in the buffers. > > > > > +->release_folio() is called when the MM wants to make a change to the > > > > > +folio that would invalidate the filesystem's private data. For example, > > > > > +it may be about to be removed from the address_space or split. The folio > > > > > +is locked and not under writeback. It may be dirty. The gfp parameter is > > > > > +not usually used for allocation, but rather to indicate what the filesystem > > > > > +may do to attempt to free the private data. The filesystem may > > > > > +return false to indicate that the folio's private data cannot be freed. > > > > > +If it returns true, it should have already removed the private data from > > > > > +the folio. If a filesystem does not provide a ->release_folio method, > > > > > +the kernel will call try_to_free_buffers(). > > > > > > > > the MM? Since you changed that above... :) > > > > > > > > With that nit fixed, > > > > Reviewed-by: Darrick J. Wong <djwong@kernel.org> > > > > > > Well, is it the MM? At this point, the decision is made by > > > filemap_release_folio(), which is the VFS, in my opinion ;-) > > > > It's in mm/filemap.c, which I think makes it squarely the pagecache/mm, > > not the vfs. > > Changed this to: > > If a filesystem does not provide a ->release_folio method, > the pagecache will assume that private data is buffer_heads and call > try_to_free_buffers(). Holy schism resolved; Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D
diff --git a/Documentation/filesystems/locking.rst b/Documentation/filesystems/locking.rst index aa1a233b0fa8..91dc9d5bc602 100644 --- a/Documentation/filesystems/locking.rst +++ b/Documentation/filesystems/locking.rst @@ -374,10 +374,16 @@ invalidate_lock before invalidating page cache in truncate / hole punch path (and thus calling into ->invalidate_folio) to block races between page cache invalidation and page cache filling functions (fault, read, ...). -->release_folio() is called when the kernel is about to try to drop the -buffers from the folio in preparation for freeing it. It returns false to -indicate that the buffers are (or may be) freeable. If ->release_folio is -NULL, the kernel assumes that the fs has no private interest in the buffers. +->release_folio() is called when the MM wants to make a change to the +folio that would invalidate the filesystem's private data. For example, +it may be about to be removed from the address_space or split. The folio +is locked and not under writeback. It may be dirty. The gfp parameter is +not usually used for allocation, but rather to indicate what the filesystem +may do to attempt to free the private data. The filesystem may +return false to indicate that the folio's private data cannot be freed. +If it returns true, it should have already removed the private data from +the folio. If a filesystem does not provide a ->release_folio method, +the kernel will call try_to_free_buffers(). ->free_folio() is called when the kernel has dropped the folio from the page cache.
The filesystem ->release_folio method is called under more circumstances now than when the documentation was written. The second sentence describing the interpretation of the return value is the wrong polarity (false indicates failure, not success). And the third sentence is also wrong (the kernel calls try_to_free_buffers() instead). So replace the entire paragraph with a detailed description of what the state of the folio may be, the meaning of the gfp parameter, why the method is being called and what the filesystem is expected to do. Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> --- Documentation/filesystems/locking.rst | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)