[PATCH 0/5] Improve buffer head documentation

[PATCH 0/5] Improve buffer head documentation

[Matthew Wilcox (Oracle)]

Having started improving the kernel-doc for __folio_mark_dirty(), I
then started noticing other improvements that could be made, and this
is as far as I got before deciding I'd made enough for now.  Tested with
make htmldocs.

Matthew Wilcox (Oracle) (5):
  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() kernel-doc
  doc: Split buffer.rst out of api-summary.rst

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

-- 
2.43.0

[PATCH 1/5] 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..96da6716cb86 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 (eg
+ * 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 2/5] 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 | 54 +++++++++++++++++++++++++++++------------------------
 1 file changed, 30 insertions(+), 24 deletions(-)

diff --git a/fs/buffer.c b/fs/buffer.c
index 5c29850e4781..31e171382e00 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -687,30 +687,36 @@ 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 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.
+ * It the folio does not have buffers then when they are later attached
+ * they will all be set 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 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.
  */
 bool block_dirty_folio(struct address_space *mapping, struct folio *folio)
 {
-- 
2.43.0

[PATCH 3/5] 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 31e171382e00..a657920802ac 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -2863,26 +2863,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 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
- * private_lock.
- *
- * try_to_free_buffers() is non-blocking.
- */
 static inline int buffer_busy(struct buffer_head *bh)
 {
 	return atomic_read(&bh->b_count) |
@@ -2916,6 +2896,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
+ * private_lock.
+ *
+ * Exclusion against try_to_free_buffers may be obtained by either
+ * locking the folio or by holding its mapping's 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 4/5] buffer: Fix __bread() 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.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 include/linux/buffer_head.h | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/include/linux/buffer_head.h b/include/linux/buffer_head.h
index d78454a4dd1f..7558cd1d3eb3 100644
--- a/include/linux/buffer_head.h
+++ b/include/linux/buffer_head.h
@@ -437,14 +437,17 @@ 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: Block size 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 buffer head has its refcount elevated and the caller
+ * should call brelse() when it has finished with the buffer.
+ *
+ * Return: NULL if the block was unreadable.
  */
 static inline struct buffer_head *
 __bread(struct block_device *bdev, sector_t block, unsigned size)
-- 
2.43.0

[PATCH 5/5] 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 09cade7eaefc..0cc2bb06de6a 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 2/5] buffer: Add kernel-doc for block_dirty_folio()

[Randy Dunlap]

On 1/4/24 08:36, 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>
> ---
>  fs/buffer.c | 54 +++++++++++++++++++++++++++++------------------------
>  1 file changed, 30 insertions(+), 24 deletions(-)
> 
> diff --git a/fs/buffer.c b/fs/buffer.c
> index 5c29850e4781..31e171382e00 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -687,30 +687,36 @@ 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 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.
> + * It the folio does not have buffers then when they are later attached
> + * they will all be set 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 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: tbd

?

>   */
>  bool block_dirty_folio(struct address_space *mapping, struct folio *folio)
>  {

-- 
#Randy

Re: [PATCH 1/5] doc: Improve the description of __folio_mark_dirty

[Randy Dunlap]

On 1/4/24 08:36, Matthew Wilcox (Oracle) wrote:
> 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..96da6716cb86 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 (eg

preferably s/eg/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)

-- 
#Randy

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

[Matthew Wilcox]

On Thu, Jan 04, 2024 at 01:06:10PM -0800, Randy Dunlap wrote:
> > +/**
> > + * 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.
> > + * It the folio does not have buffers then when they are later attached
> > + * they will all be set 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 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: tbd

+ *
+ * Return: True if the folio was dirtied; false if it was already dirtied.

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

[Pankaj Raghav (Samsung)]

> + * If the folio has buffers, the uptodate buffers are set dirty, to
> + * preserve dirty-state coherency between the folio and the buffers.
> + * It the folio does not have buffers then when they are later attached

s/It the folio/If the folio
> + * they will all be set dirty.
Is it better to rephrase it slightly as follows:

If the folio does not have buffers, they will all be set dirty when they
are later attached.

> + *
> + * 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

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

[Matthew Wilcox]

On Mon, Jan 08, 2024 at 02:31:17PM +0100, Pankaj Raghav (Samsung) wrote:
> > + * If the folio has buffers, the uptodate buffers are set dirty, to
> > + * preserve dirty-state coherency between the folio and the buffers.
> > + * It the folio does not have buffers then when they are later attached
> 
> s/It the folio/If the folio
> > + * they will all be set dirty.
> Is it better to rephrase it slightly as follows:
> 
> If the folio does not have buffers, they will all be set dirty when they
> are later attached.

Yes, I like that better.

> > + *
> > + * 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
> 

Re: [PATCH 4/5] buffer: Fix __bread() kernel-doc

[Pankaj Raghav (Samsung)]

On Thu, Jan 04, 2024 at 04:36:51PM +0000, Matthew Wilcox (Oracle) wrote:
> 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.
> 
It looks like __bread_gfp has the same problem:

diff --git a/fs/buffer.c b/fs/buffer.c
index 967f34b70aa8..cfdf45cc290a 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -1446,16 +1446,18 @@ 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
+ * __bread_gfp() - Read a block.
+ * @bdev: The block device to read from.
+ * @block: Block number in units of block size.
+ * @size: Block size in bytes.
  *
- *  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.
+ * Read a specified block, and return the buffer head that refers to it.
+ * The memory can be allocated from a non-movable area to not to prevent
+ * page migration if you set gfp to zero. The buffer head has its
+ * refcount elevated and the caller should call brelse() when it has
+ * finished with the buffer.
+ *
+ * Return: NULL if the block was unreadable.
  */
 struct buffer_head *
 __bread_gfp(struct block_device *bdev, sector_t block,
(END)

Another option is to just change this in __bread_gfp() and add a See
__bread_gfp() in __bread()?

Re: [PATCH 4/5] buffer: Fix __bread() kernel-doc

[Matthew Wilcox]

On Mon, Jan 08, 2024 at 03:58:08PM +0100, Pankaj Raghav (Samsung) wrote:
> On Thu, Jan 04, 2024 at 04:36:51PM +0000, Matthew Wilcox (Oracle) wrote:
> > 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.
> > 
> It looks like __bread_gfp has the same problem:

I'm happy to incorporate this patch, but I'll need your S-o-B on it.

> diff --git a/fs/buffer.c b/fs/buffer.c
> index 967f34b70aa8..cfdf45cc290a 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -1446,16 +1446,18 @@ 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
> + * __bread_gfp() - Read a block.
> + * @bdev: The block device to read from.
> + * @block: Block number in units of block size.
> + * @size: Block size in bytes.
>   *
> - *  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.
> + * Read a specified block, and return the buffer head that refers to it.
> + * The memory can be allocated from a non-movable area to not to prevent
> + * page migration if you set gfp to zero. The buffer head has its
> + * refcount elevated and the caller should call brelse() when it has
> + * finished with the buffer.
> + *
> + * Return: NULL if the block was unreadable.
>   */
>  struct buffer_head *
>  __bread_gfp(struct block_device *bdev, sector_t block,
> (END)
> 
> Another option is to just change this in __bread_gfp() and add a See
> __bread_gfp() in __bread()?

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

[Matthew Wilcox]

On Mon, Jan 08, 2024 at 01:35:10PM +0000, Matthew Wilcox wrote:
> On Mon, Jan 08, 2024 at 02:31:17PM +0100, Pankaj Raghav (Samsung) wrote:
> > > + * If the folio has buffers, the uptodate buffers are set dirty, to
> > > + * preserve dirty-state coherency between the folio and the buffers.
> > > + * It the folio does not have buffers then when they are later attached
> > 
> > s/It the folio/If the folio
> > > + * they will all be set dirty.
> > Is it better to rephrase it slightly as follows:
> > 
> > If the folio does not have buffers, they will all be set dirty when they
> > are later attached.
> 
> Yes, I like that better.

Actually, how about:

 * 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.

I considered deleting the sentence entirely as it's not actually related
to what the function does; it's just a note about how the buffer cache
behaves.  That said, information about how buffer heds work is scant
enough that I don't want to delete it.

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

[Pankaj Raghav (Samsung)]

> Actually, how about:
> 
>  * 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.

This looks good to me :)

> 
> I considered deleting the sentence entirely as it's not actually related
> to what the function does; it's just a note about how the buffer cache
> behaves.  That said, information about how buffer heds work is scant
> enough that I don't want to delete it.

Re: [PATCH 4/5] buffer: Fix __bread() kernel-doc

[Pankaj Raghav (Samsung)]

On Mon, Jan 08, 2024 at 04:09:01PM +0000, Matthew Wilcox wrote:
> On Mon, Jan 08, 2024 at 03:58:08PM +0100, Pankaj Raghav (Samsung) wrote:
> > On Thu, Jan 04, 2024 at 04:36:51PM +0000, Matthew Wilcox (Oracle) wrote:
> > > 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.
> > > 
> > It looks like __bread_gfp has the same problem:
> 
> I'm happy to incorporate this patch, but I'll need your S-o-B on it.

Something like this:

From: "Matthew Wilcox (Oracle)" <willy@infradead.org>
Date: Mon, 8 Jan 2024 19:37:41 +0100
Subject: [PATCH] buffer: Update __bread() and __bread_gfp kernel-doc

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.

Instead of duplicating the doc in __bread() and __bread_gfp(), update
__bread_gfp() doc and point to it from __bread().

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

diff --git a/fs/buffer.c b/fs/buffer.c
index 967f34b70aa8..ea55fb3fcfae 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -1446,16 +1446,19 @@ 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
+ * __bread_gfp() - Read a block.
+ * @bdev: The block device to read from.
+ * @block: Block number in units of block size.
+ * @size: Block size in bytes.
+ * @gfp: gfp flags.
  *
- *  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.
+ * Read a specified block, and return the buffer head that refers to it.
+ * The memory can be allocated from a non-movable area to not to prevent
+ * page migration if you set gfp to zero. The buffer head has its
+ * refcount elevated and the caller should call brelse() when it has
+ * finished with the buffer.
+ *
+ * Return: NULL if the block was unreadable.
  */
 struct buffer_head *
 __bread_gfp(struct block_device *bdev, sector_t block,
diff --git a/include/linux/buffer_head.h b/include/linux/buffer_head.h
index 5f23ee599889..ac56014b29dd 100644
--- a/include/linux/buffer_head.h
+++ b/include/linux/buffer_head.h
@@ -440,14 +440,7 @@ 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
- *
- *  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.
+ * See __bread_gfp()
  */
 static inline struct buffer_head *
 __bread(struct block_device *bdev, sector_t block, unsigned size)
-- 
2.40.1