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
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) <[email protected]>
---
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
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) <[email protected]>
---
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
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) <[email protected]>
---
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
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) <[email protected]>
---
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
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) <[email protected]>
---
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
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) <[email protected]>
> ---
> 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
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) <[email protected]>
> ---
> 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
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.
> + * 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
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
>
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()?
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()?
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.
> 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.
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)" <[email protected]>
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)" <[email protected]>
Signed-off-by: Pankaj Raghav <[email protected]>
---
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