[PATCH v2 0/8] Improve buffer head documentation

[PATCH v2 0/8] Improve buffer head documentation

[Matthew Wilcox (Oracle)]

Turn buffer head documentation into its own document, and make many
general improvements to the docs.  Obviously there is much more that
could be done.  Tested with make htmldocs.

v2:
 - Incorporate feedback from Randy & Pankaj
 - Add docs for brelse() and bforget()
 - Improve bdev_getblk() docs

Matthew Wilcox (Oracle) (8):
  doc: Improve the description of __folio_mark_dirty
  buffer: Add kernel-doc for block_dirty_folio()
  buffer: Add kernel-doc for try_to_free_buffers()
  buffer: Fix __bread and __bread_gfp kernel-doc
  buffer: Add kernel-doc for brelse() and __brelse()
  buffer: Add kernel-doc for bforget() and __bforget()
  buffer: Improve bdev_getblk documentation
  doc: Split buffer.rst out of api-summary.rst

 Documentation/filesystems/api-summary.rst |   3 -
 Documentation/filesystems/index.rst       |   1 +
 fs/buffer.c                               | 165 +++++++++++++---------
 include/linux/buffer_head.h               |  48 +++++--
 mm/page-writeback.c                       |  14 +-
 5 files changed, 145 insertions(+), 86 deletions(-)

-- 
2.43.0

[PATCH v2 1/8] doc: Improve the description of __folio_mark_dirty

[Matthew Wilcox (Oracle)]

I've learned why it's safe to call __folio_mark_dirty() from
mark_buffer_dirty() without holding the folio lock, so update
the description to explain why.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 mm/page-writeback.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/mm/page-writeback.c b/mm/page-writeback.c
index cd4e4ae77c40..f09179fca2cf 100644
--- a/mm/page-writeback.c
+++ b/mm/page-writeback.c
@@ -2652,11 +2652,15 @@ void folio_account_cleaned(struct folio *folio, struct bdi_writeback *wb)
  * If warn is true, then emit a warning if the folio is not uptodate and has
  * not been truncated.
  *
- * The caller must hold folio_memcg_lock().  Most callers have the folio
- * locked.  A few have the folio blocked from truncation through other
- * means (eg zap_vma_pages() has it mapped and is holding the page table
- * lock).  This can also be called from mark_buffer_dirty(), which I
- * cannot prove is always protected against truncate.
+ * The caller must hold folio_memcg_lock().  It is the caller's
+ * responsibility to prevent the folio from being truncated while
+ * this function is in progress, although it may have been truncated
+ * before this function is called.  Most callers have the folio locked.
+ * A few have the folio blocked from truncation through other means (e.g.
+ * zap_vma_pages() has it mapped and is holding the page table lock).
+ * When called from mark_buffer_dirty(), the filesystem should hold a
+ * reference to the buffer_head that is being marked dirty, which causes
+ * try_to_free_buffers() to fail.
  */
 void __folio_mark_dirty(struct folio *folio, struct address_space *mapping,
 			     int warn)
-- 
2.43.0

[PATCH v2 2/8] buffer: Add kernel-doc for block_dirty_folio()

[Matthew Wilcox (Oracle)]

Turn the excellent documentation for this function into kernel-doc.
Replace 'page' with 'folio' and make a few other minor updates.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/buffer.c | 55 ++++++++++++++++++++++++++++++-----------------------
 1 file changed, 31 insertions(+), 24 deletions(-)

diff --git a/fs/buffer.c b/fs/buffer.c
index d3bcf601d3e5..071f01b28c90 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -687,30 +687,37 @@ void mark_buffer_dirty_inode(struct buffer_head *bh, struct inode *inode)
 }
 EXPORT_SYMBOL(mark_buffer_dirty_inode);
 
-/*
- * Add a page to the dirty page list.
- *
- * It is a sad fact of life that this function is called from several places
- * deeply under spinlocking.  It may not sleep.
- *
- * If the page has buffers, the uptodate buffers are set dirty, to preserve
- * dirty-state coherency between the page and the buffers.  It the page does
- * not have buffers then when they are later attached they will all be set
- * dirty.
- *
- * The buffers are dirtied before the page is dirtied.  There's a small race
- * window in which a writepage caller may see the page cleanness but not the
- * buffer dirtiness.  That's fine.  If this code were to set the page dirty
- * before the buffers, a concurrent writepage caller could clear the page dirty
- * bit, see a bunch of clean buffers and we'd end up with dirty buffers/clean
- * page on the dirty page list.
- *
- * We use i_private_lock to lock against try_to_free_buffers while using the
- * page's buffer list.  Also use this to protect against clean buffers being
- * added to the page after it was set dirty.
- *
- * FIXME: may need to call ->reservepage here as well.  That's rather up to the
- * address_space though.
+/**
+ * block_dirty_folio - Mark a folio as dirty.
+ * @mapping: The address space containing this folio.
+ * @folio: The folio to mark dirty.
+ *
+ * Filesystems which use buffer_heads can use this function as their
+ * ->dirty_folio implementation.  Some filesystems need to do a little
+ * work before calling this function.  Filesystems which do not use
+ * buffer_heads should call filemap_dirty_folio() instead.
+ *
+ * If the folio has buffers, the uptodate buffers are set dirty, to
+ * preserve dirty-state coherency between the folio and the buffers.
+ * Buffers added to a dirty folio are created dirty.
+ *
+ * The buffers are dirtied before the folio is dirtied.  There's a small
+ * race window in which writeback may see the folio cleanness but not the
+ * buffer dirtiness.  That's fine.  If this code were to set the folio
+ * dirty before the buffers, writeback could clear the folio dirty flag,
+ * see a bunch of clean buffers and we'd end up with dirty buffers/clean
+ * folio on the dirty folio list.
+ *
+ * We use i_private_lock to lock against try_to_free_buffers() while
+ * using the folio's buffer list.  This also prevents clean buffers
+ * being added to the folio after it was set dirty.
+ *
+ * Context: May only be called from process context.  Does not sleep.
+ * Caller must ensure that @folio cannot be truncated during this call,
+ * typically by holding the folio lock or having a page in the folio
+ * mapped and holding the page table lock.
+ *
+ * Return: True if the folio was dirtied; false if it was already dirtied.
  */
 bool block_dirty_folio(struct address_space *mapping, struct folio *folio)
 {
-- 
2.43.0

[PATCH v2 3/8] buffer: Add kernel-doc for try_to_free_buffers()

[Matthew Wilcox (Oracle)]

The documentation for this function has become separated from it over
time; move it to the right place and turn it into kernel-doc.  Mild
editing of the content to make it more about what the function does, and
less about how it does it.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/buffer.c | 44 ++++++++++++++++++++++++--------------------
 1 file changed, 24 insertions(+), 20 deletions(-)

diff --git a/fs/buffer.c b/fs/buffer.c
index 071f01b28c90..25861241657f 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -2864,26 +2864,6 @@ int sync_dirty_buffer(struct buffer_head *bh)
 }
 EXPORT_SYMBOL(sync_dirty_buffer);
 
-/*
- * try_to_free_buffers() checks if all the buffers on this particular folio
- * are unused, and releases them if so.
- *
- * Exclusion against try_to_free_buffers may be obtained by either
- * locking the folio or by holding its mapping's i_private_lock.
- *
- * If the folio is dirty but all the buffers are clean then we need to
- * be sure to mark the folio clean as well.  This is because the folio
- * may be against a block device, and a later reattachment of buffers
- * to a dirty folio will set *all* buffers dirty.  Which would corrupt
- * filesystem data on the same device.
- *
- * The same applies to regular filesystem folios: if all the buffers are
- * clean then we set the folio clean and proceed.  To do that, we require
- * total exclusion from block_dirty_folio().  That is obtained with
- * i_private_lock.
- *
- * try_to_free_buffers() is non-blocking.
- */
 static inline int buffer_busy(struct buffer_head *bh)
 {
 	return atomic_read(&bh->b_count) |
@@ -2917,6 +2897,30 @@ drop_buffers(struct folio *folio, struct buffer_head **buffers_to_free)
 	return false;
 }
 
+/**
+ * try_to_free_buffers: Release buffers attached to this folio.
+ * @folio: The folio.
+ *
+ * If any buffers are in use (dirty, under writeback, elevated refcount),
+ * no buffers will be freed.
+ *
+ * If the folio is dirty but all the buffers are clean then we need to
+ * be sure to mark the folio clean as well.  This is because the folio
+ * may be against a block device, and a later reattachment of buffers
+ * to a dirty folio will set *all* buffers dirty.  Which would corrupt
+ * filesystem data on the same device.
+ *
+ * The same applies to regular filesystem folios: if all the buffers are
+ * clean then we set the folio clean and proceed.  To do that, we require
+ * total exclusion from block_dirty_folio().  That is obtained with
+ * i_private_lock.
+ *
+ * Exclusion against try_to_free_buffers may be obtained by either
+ * locking the folio or by holding its mapping's i_private_lock.
+ *
+ * Context: Process context.  @folio must be locked.  Will not sleep.
+ * Return: true if all buffers attached to this folio were freed.
+ */
 bool try_to_free_buffers(struct folio *folio)
 {
 	struct address_space * const mapping = folio->mapping;
-- 
2.43.0

[PATCH v2 4/8] buffer: Fix __bread and __bread_gfp kernel-doc

[Matthew Wilcox (Oracle)]

The extra indentation confused the kernel-doc parser, so remove it.
Fix some other wording while I'm here, and advise the user they need to
call brelse() on this buffer.

__bread_gfp() isn't used directly by filesystems, but the other wrappers
for it don't have documentation, so document it accordingly.

Co-developed-by: Pankaj Raghav <p.raghav@samsung.com>
Signed-off-by: Pankaj Raghav <p.raghav@samsung.com>
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/buffer.c                 | 35 ++++++++++++++++++++++-------------
 include/linux/buffer_head.h | 22 +++++++++++++---------
 2 files changed, 35 insertions(+), 22 deletions(-)

diff --git a/fs/buffer.c b/fs/buffer.c
index 25861241657f..160bbc1f929c 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -1453,20 +1453,29 @@ void __breadahead(struct block_device *bdev, sector_t block, unsigned size)
 EXPORT_SYMBOL(__breadahead);
 
 /**
- *  __bread_gfp() - reads a specified block and returns the bh
- *  @bdev: the block_device to read from
- *  @block: number of block
- *  @size: size (in bytes) to read
- *  @gfp: page allocation flag
- *
- *  Reads a specified block, and returns buffer head that contains it.
- *  The page cache can be allocated from non-movable area
- *  not to prevent page migration if you set gfp to zero.
- *  It returns NULL if the block was unreadable.
+ * __bread_gfp() - Read a block.
+ * @bdev: The block device to read from.
+ * @block: Block number in units of block size.
+ * @size: The block size of this device in bytes.
+ * @gfp: Not page allocation flags; see below.
+ *
+ * You are not expected to call this function.  You should use one of
+ * sb_bread(), sb_bread_unmovable() or __bread().
+ *
+ * Read a specified block, and return the buffer head that refers to it.
+ * If @gfp is 0, the memory will be allocated using the block device's
+ * default GFP flags.  If @gfp is __GFP_MOVABLE, the memory may be
+ * allocated from a movable area.  Do not pass in a complete set of
+ * GFP flags.
+ *
+ * The returned buffer head has its refcount increased.  The caller should
+ * call brelse() when it has finished with the buffer.
+ *
+ * Context: May sleep waiting for I/O.
+ * Return: NULL if the block was unreadable.
  */
-struct buffer_head *
-__bread_gfp(struct block_device *bdev, sector_t block,
-		   unsigned size, gfp_t gfp)
+struct buffer_head *__bread_gfp(struct block_device *bdev, sector_t block,
+		unsigned size, gfp_t gfp)
 {
 	struct buffer_head *bh;
 
diff --git a/include/linux/buffer_head.h b/include/linux/buffer_head.h
index d78454a4dd1f..56a1e9c1e71e 100644
--- a/include/linux/buffer_head.h
+++ b/include/linux/buffer_head.h
@@ -437,17 +437,21 @@ static inline void bh_readahead_batch(int nr, struct buffer_head *bhs[],
 }
 
 /**
- *  __bread() - reads a specified block and returns the bh
- *  @bdev: the block_device to read from
- *  @block: number of block
- *  @size: size (in bytes) to read
+ * __bread() - Read a block.
+ * @bdev: The block device to read from.
+ * @block: Block number in units of block size.
+ * @size: The block size of this device in bytes.
  *
- *  Reads a specified block, and returns buffer head that contains it.
- *  The page cache is allocated from movable area so that it can be migrated.
- *  It returns NULL if the block was unreadable.
+ * Read a specified block, and return the buffer head that refers
+ * to it.  The memory is allocated from the movable area so that it can
+ * be migrated.  The returned buffer head has its refcount increased.
+ * The caller should call brelse() when it has finished with the buffer.
+ *
+ * Context: May sleep waiting for I/O.
+ * Return: NULL if the block was unreadable.
  */
-static inline struct buffer_head *
-__bread(struct block_device *bdev, sector_t block, unsigned size)
+static inline struct buffer_head *__bread(struct block_device *bdev,
+		sector_t block, unsigned size)
 {
 	return __bread_gfp(bdev, block, size, __GFP_MOVABLE);
 }
-- 
2.43.0

[PATCH v2 5/8] buffer: Add kernel-doc for brelse() and __brelse()

[Matthew Wilcox (Oracle)]

Move the documentation for __brelse() to brelse(), format it as
kernel-doc and update it from talking about pages to folios.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/buffer.c                 | 17 ++++++++---------
 include/linux/buffer_head.h | 16 ++++++++++++++++
 2 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/fs/buffer.c b/fs/buffer.c
index 160bbc1f929c..9a7b3649c872 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -1226,17 +1226,16 @@ void mark_buffer_write_io_error(struct buffer_head *bh)
 }
 EXPORT_SYMBOL(mark_buffer_write_io_error);
 
-/*
- * Decrement a buffer_head's reference count.  If all buffers against a page
- * have zero reference count, are clean and unlocked, and if the page is clean
- * and unlocked then try_to_free_buffers() may strip the buffers from the page
- * in preparation for freeing it (sometimes, rarely, buffers are removed from
- * a page but it ends up not being freed, and buffers may later be reattached).
+/**
+ * __brelse - Release a buffer.
+ * @bh: The buffer to release.
+ *
+ * This variant of brelse() can be called if @bh is guaranteed to not be NULL.
  */
-void __brelse(struct buffer_head * buf)
+void __brelse(struct buffer_head *bh)
 {
-	if (atomic_read(&buf->b_count)) {
-		put_bh(buf);
+	if (atomic_read(&bh->b_count)) {
+		put_bh(bh);
 		return;
 	}
 	WARN(1, KERN_ERR "VFS: brelse: Trying to free free buffer\n");
diff --git a/include/linux/buffer_head.h b/include/linux/buffer_head.h
index 56a1e9c1e71e..3cbc01bbc398 100644
--- a/include/linux/buffer_head.h
+++ b/include/linux/buffer_head.h
@@ -303,6 +303,22 @@ static inline void put_bh(struct buffer_head *bh)
         atomic_dec(&bh->b_count);
 }
 
+/**
+ * brelse - Release a buffer.
+ * @bh: The buffer to release.
+ *
+ * Decrement a buffer_head's reference count.  If @bh is NULL, this
+ * function is a no-op.
+ *
+ * If all buffers on a folio have zero reference count, are clean
+ * and unlocked, and if the folio is clean and unlocked then
+ * try_to_free_buffers() may strip the buffers from the folio in
+ * preparation for freeing it (sometimes, rarely, buffers are removed
+ * from a folio but it ends up not being freed, and buffers may later
+ * be reattached).
+ *
+ * Context: Any context.
+ */
 static inline void brelse(struct buffer_head *bh)
 {
 	if (bh)
-- 
2.43.0

[PATCH v2 6/8] buffer: Add kernel-doc for bforget() and __bforget()

[Matthew Wilcox (Oracle)]

Distinguish these functions from brelse() and __brelse().

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/buffer.c                 |  9 ++++++---
 include/linux/buffer_head.h | 10 ++++++++++
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/fs/buffer.c b/fs/buffer.c
index 9a7b3649c872..05b68eccfdcc 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -1242,9 +1242,12 @@ void __brelse(struct buffer_head *bh)
 }
 EXPORT_SYMBOL(__brelse);
 
-/*
- * bforget() is like brelse(), except it discards any
- * potentially dirty data.
+/**
+ * __bforget - Discard any dirty data in a buffer.
+ * @bh: The buffer to forget.
+ *
+ * This variant of bforget() can be called if @bh is guaranteed to not
+ * be NULL.
  */
 void __bforget(struct buffer_head *bh)
 {
diff --git a/include/linux/buffer_head.h b/include/linux/buffer_head.h
index 3cbc01bbc398..9dc5477f13c7 100644
--- a/include/linux/buffer_head.h
+++ b/include/linux/buffer_head.h
@@ -325,6 +325,16 @@ static inline void brelse(struct buffer_head *bh)
 		__brelse(bh);
 }
 
+/**
+ * bforget - Discard any dirty data in a buffer.
+ * @bh: The buffer to forget.
+ *
+ * Call this function instead of brelse() if the data written to a buffer
+ * no longer needs to be written back.  It will clear the buffer's dirty
+ * flag so writeback of this buffer will be skipped.
+ *
+ * Context: Any context.
+ */
 static inline void bforget(struct buffer_head *bh)
 {
 	if (bh)
-- 
2.43.0

[PATCH v2 7/8] buffer: Improve bdev_getblk documentation

[Matthew Wilcox (Oracle)]

Add some more information about the state of the buffer_head returned.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/buffer.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/fs/buffer.c b/fs/buffer.c
index 05b68eccfdcc..562de7e013f7 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -1424,6 +1424,11 @@ EXPORT_SYMBOL(__find_get_block);
  * @size: The size of buffer_heads for this @bdev.
  * @gfp: The memory allocation flags to use.
  *
+ * The returned buffer head has its reference count incremented, but is
+ * not locked.  The caller should call brelse() when it has finished
+ * with the buffer.  The buffer may not be uptodate.  If needed, the
+ * caller can bring it uptodate either by reading it or overwriting it.
+ *
  * Return: The buffer head, or NULL if memory could not be allocated.
  */
 struct buffer_head *bdev_getblk(struct block_device *bdev, sector_t block,
-- 
2.43.0

[PATCH v2 8/8] doc: Split buffer.rst out of api-summary.rst

[Matthew Wilcox (Oracle)]

Buffer heads are no longer a generic filesystem API but an optional
filesystem support library.  Make the documentation structure reflect
that, and include the fine documentation kept in buffer_head.h.
We could give a better overview of what buffer heads are all about,
but my enthusiasm for documenting it is limited.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 Documentation/filesystems/api-summary.rst | 3 ---
 Documentation/filesystems/index.rst       | 1 +
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/Documentation/filesystems/api-summary.rst b/Documentation/filesystems/api-summary.rst
index 98db2ea5fa12..cc5cc7f3fbd8 100644
--- a/Documentation/filesystems/api-summary.rst
+++ b/Documentation/filesystems/api-summary.rst
@@ -56,9 +56,6 @@ Other Functions
 .. kernel-doc:: fs/namei.c
    :export:
 
-.. kernel-doc:: fs/buffer.c
-   :export:
-
 .. kernel-doc:: block/bio.c
    :export:
 
diff --git a/Documentation/filesystems/index.rst b/Documentation/filesystems/index.rst
index e18bc5ae3b35..5fc9b19b8d8e 100644
--- a/Documentation/filesystems/index.rst
+++ b/Documentation/filesystems/index.rst
@@ -50,6 +50,7 @@ filesystem implementations.
 .. toctree::
    :maxdepth: 2
 
+   buffer
    journalling
    fscrypt
    fsverity
-- 
2.43.0

Re: [PATCH v2 2/8] buffer: Add kernel-doc for block_dirty_folio()

[Pankaj Raghav (Samsung)]

On Tue, Jan 09, 2024 at 02:33:51PM +0000, Matthew Wilcox (Oracle) wrote:
> Turn the excellent documentation for this function into kernel-doc.
> Replace 'page' with 'folio' and make a few other minor updates.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>

Looks good,
Reviewed-by: Pankaj Raghav <p.raghav@samsung.com>

Re: [PATCH v2 3/8] buffer: Add kernel-doc for try_to_free_buffers()

[Pankaj Raghav (Samsung)]

On Tue, Jan 09, 2024 at 02:33:52PM +0000, Matthew Wilcox (Oracle) wrote:
> The documentation for this function has become separated from it over
> time; move it to the right place and turn it into kernel-doc.  Mild
> editing of the content to make it more about what the function does, and
> less about how it does it.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>

Looks good,
Reviewed-by: Pankaj Raghav <p.raghav@samsung.com>

Re: [PATCH v2 5/8] buffer: Add kernel-doc for brelse() and __brelse()

[Pankaj Raghav (Samsung)]

> + * If all buffers on a folio have zero reference count, are clean
> + * and unlocked, and if the folio is clean and unlocked then

IIUC from your [PATCH 3/8], folio only needs to be unlocked to free the
buffers as try_to_free_buffers() will remove the dirty flag and "clean"
the folio?
So:
s/if folio is clean and unlocked/if folio is unlocked

> + * try_to_free_buffers() may strip the buffers from the folio in
> + * preparation for freeing it (sometimes, rarely, buffers are removed
> + * from a folio but it ends up not being freed, and buffers may later
> + * be reattached).
> + *
> + * Context: Any context.
> + */
>  static inline void brelse(struct buffer_head *bh)
>  {
>  	if (bh)
> -- 
> 2.43.0
> 

Re: [PATCH v2 5/8] buffer: Add kernel-doc for brelse() and __brelse()

[Matthew Wilcox]

On Wed, Jan 10, 2024 at 03:30:54PM +0100, Pankaj Raghav (Samsung) wrote:
> > + * If all buffers on a folio have zero reference count, are clean
> > + * and unlocked, and if the folio is clean and unlocked then
> 
> IIUC from your [PATCH 3/8], folio only needs to be unlocked to free the
> buffers as try_to_free_buffers() will remove the dirty flag and "clean"
> the folio?
> So:
> s/if folio is clean and unlocked/if folio is unlocked

That's a good point.  Perhaps "unlocked and not under writeback"
would be better wording, since that would be true.

Re: [PATCH v2 5/8] buffer: Add kernel-doc for brelse() and __brelse()

[Pankaj Raghav (Samsung)]

On Wed, Jan 10, 2024 at 05:26:55PM +0000, Matthew Wilcox wrote:
> On Wed, Jan 10, 2024 at 03:30:54PM +0100, Pankaj Raghav (Samsung) wrote:
> > > + * If all buffers on a folio have zero reference count, are clean
> > > + * and unlocked, and if the folio is clean and unlocked then
> > 
> > IIUC from your [PATCH 3/8], folio only needs to be unlocked to free the
> > buffers as try_to_free_buffers() will remove the dirty flag and "clean"
> > the folio?
> > So:
> > s/if folio is clean and unlocked/if folio is unlocked
> 
> That's a good point.  Perhaps "unlocked and not under writeback"
> would be better wording, since that would be true.

Yeah. That sounds good to me!

Re: [PATCH v2 3/8] buffer: Add kernel-doc for try_to_free_buffers()

[Randy Dunlap]

Hi,

On 1/9/24 06:33, Matthew Wilcox (Oracle) wrote:
> The documentation for this function has become separated from it over
> time; move it to the right place and turn it into kernel-doc.  Mild
> editing of the content to make it more about what the function does, and
> less about how it does it.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>  fs/buffer.c | 44 ++++++++++++++++++++++++--------------------
>  1 file changed, 24 insertions(+), 20 deletions(-)
> 
> diff --git a/fs/buffer.c b/fs/buffer.c
> index 071f01b28c90..25861241657f 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -2864,26 +2864,6 @@ int sync_dirty_buffer(struct buffer_head *bh)
>  }
>  EXPORT_SYMBOL(sync_dirty_buffer);
>  
> -/*
> - * try_to_free_buffers() checks if all the buffers on this particular folio
> - * are unused, and releases them if so.
> - *
> - * Exclusion against try_to_free_buffers may be obtained by either
> - * locking the folio or by holding its mapping's i_private_lock.
> - *
> - * If the folio is dirty but all the buffers are clean then we need to
> - * be sure to mark the folio clean as well.  This is because the folio
> - * may be against a block device, and a later reattachment of buffers
> - * to a dirty folio will set *all* buffers dirty.  Which would corrupt
> - * filesystem data on the same device.
> - *
> - * The same applies to regular filesystem folios: if all the buffers are
> - * clean then we set the folio clean and proceed.  To do that, we require
> - * total exclusion from block_dirty_folio().  That is obtained with
> - * i_private_lock.
> - *
> - * try_to_free_buffers() is non-blocking.
> - */
>  static inline int buffer_busy(struct buffer_head *bh)
>  {
>  	return atomic_read(&bh->b_count) |
> @@ -2917,6 +2897,30 @@ drop_buffers(struct folio *folio, struct buffer_head **buffers_to_free)
>  	return false;
>  }
>  
> +/**
> + * try_to_free_buffers: Release buffers attached to this folio.

preferably s/_buffers: /_buffers - /

> + * @folio: The folio.
> + *
> + * If any buffers are in use (dirty, under writeback, elevated refcount),
> + * no buffers will be freed.
> + *
> + * If the folio is dirty but all the buffers are clean then we need to
> + * be sure to mark the folio clean as well.  This is because the folio
> + * may be against a block device, and a later reattachment of buffers
> + * to a dirty folio will set *all* buffers dirty.  Which would corrupt
> + * filesystem data on the same device.
> + *
> + * The same applies to regular filesystem folios: if all the buffers are
> + * clean then we set the folio clean and proceed.  To do that, we require
> + * total exclusion from block_dirty_folio().  That is obtained with
> + * i_private_lock.
> + *
> + * Exclusion against try_to_free_buffers may be obtained by either
> + * locking the folio or by holding its mapping's i_private_lock.
> + *
> + * Context: Process context.  @folio must be locked.  Will not sleep.
> + * Return: true if all buffers attached to this folio were freed.
> + */
>  bool try_to_free_buffers(struct folio *folio)
>  {
>  	struct address_space * const mapping = folio->mapping;

-- 
#Randy