* Re: [PATCH 01/23] btrfs: pass a flags argument to cow_file_range
@ 2023-06-29 2:34 Naohiro Aota
0 siblings, 0 replies; 5+ messages in thread
From: Naohiro Aota @ 2023-06-29 2:34 UTC (permalink / raw)
To: Christoph Hellwig, Chris Mason, Josef Bacik, David Sterba
Cc: Matthew Wilcox, linux-btrfs, linux-fsdevel
Christoph Hellwig <hch@lst.de> writes:
> The int used as bool unlock is not a very good way to describe the
> behavior, and the next patch will have to add another beahvior modifier.
> Switch to pass a flag instead, with an inital CFR_KEEP_LOCKED flag that
> specifies the pages should always be kept locked. This is the inverse
> of the old unlock argument for the reason that it requires a flag for
> the exceptional behavior.
Yeah, I always struggled to remember which is the "1" means for, lock or
unlock. So, giving it a name is really nice.
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> fs/btrfs/inode.c | 51 ++++++++++++++++++++++--------------------------
> 1 file changed, 23 insertions(+), 28 deletions(-)
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index dbbb67293e345c..92a78940991fcb 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -124,11 +124,13 @@ static struct kmem_cache *btrfs_inode_cachep;
>
> static int btrfs_setsize(struct inode *inode, struct iattr *attr);
> static int btrfs_truncate(struct btrfs_inode *inode, bool skip_writeback);
> +
> +#define CFR_KEEP_LOCKED (1 << 0)
> static noinline int cow_file_range(struct btrfs_inode *inode,
> struct page *locked_page,
> u64 start, u64 end, int *page_started,
> - unsigned long *nr_written, int unlock,
> - u64 *done_offset);
> + unsigned long *nr_written, u64 *done_offset,
> + u32 flags);
> static struct extent_map *create_io_em(struct btrfs_inode *inode, u64 start,
> u64 len, u64 orig_start, u64 block_start,
> u64 block_len, u64 orig_block_len,
> @@ -1148,7 +1150,7 @@ static int submit_uncompressed_range(struct btrfs_inode *inode,
> * can directly submit them without interruption.
> */
> ret = cow_file_range(inode, locked_page, start, end, &page_started,
> - &nr_written, 0, NULL);
> + &nr_written, NULL, CFR_KEEP_LOCKED);
> /* Inline extent inserted, page gets unlocked and everything is done */
> if (page_started)
> return 0;
> @@ -1362,25 +1364,18 @@ static u64 get_extent_allocation_hint(struct btrfs_inode *inode, u64 start,
> * locked_page is the page that writepage had locked already. We use
> * it to make sure we don't do extra locks or unlocks.
> *
> - * *page_started is set to one if we unlock locked_page and do everything
> - * required to start IO on it. It may be clean and already done with
> - * IO when we return.
> - *
> - * When unlock == 1, we unlock the pages in successfully allocated regions.
> - * When unlock == 0, we leave them locked for writing them out.
> + * When this function fails, it unlocks all pages except @locked_page.
> *
> - * However, we unlock all the pages except @locked_page in case of failure.
> + * When this function successfully creates an inline extent, it sets page_started
> + * to 1 and unlocks all pages including locked_page and starts I/O on them.
nit: @locked_page for the consistency.
> + * (In reality inline extents are limited to a single page, so locked_page is
Same here.
Other than that, looks good to me.
Reviewed-by: Naohiro Aota <naohiro.aota@wdc.com>
> + * the only page handled anyway).
> *
> - * In summary, page locking state will be as follow:
> + * When this function succeed and creates a normal extent, the page locking
> + * status depends on the passed in flags:
> *
> - * - page_started == 1 (return value)
> - * - All the pages are unlocked. IO is started.
> - * - Note that this can happen only on success
> - * - unlock == 1
> - * - All the pages except @locked_page are unlocked in any case
> - * - unlock == 0
> - * - On success, all the pages are locked for writing out them
> - * - On failure, all the pages except @locked_page are unlocked
> + * - If CFR_KEEP_LOCKED is set, all pages are kept locked.
> + * - Else all pages except for @locked_page are unlocked.
> *
> * When a failure happens in the second or later iteration of the
> * while-loop, the ordered extents created in previous iterations are kept
> @@ -1391,8 +1386,8 @@ static u64 get_extent_allocation_hint(struct btrfs_inode *inode, u64 start,
> static noinline int cow_file_range(struct btrfs_inode *inode,
> struct page *locked_page,
> u64 start, u64 end, int *page_started,
> - unsigned long *nr_written, int unlock,
> - u64 *done_offset)
> + unsigned long *nr_written, u64 *done_offset,
> + u32 flags)
> {
> struct btrfs_root *root = inode->root;
> struct btrfs_fs_info *fs_info = root->fs_info;
> @@ -1558,7 +1553,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
> * Do set the Ordered (Private2) bit so we know this page was
> * properly setup for writepage.
> */
> - page_ops = unlock ? PAGE_UNLOCK : 0;
> + page_ops = (flags & CFR_KEEP_LOCKED) ? 0 : PAGE_UNLOCK;
> page_ops |= PAGE_SET_ORDERED;
>
> extent_clear_unlock_delalloc(inode, start, start + ram_size - 1,
> @@ -1627,10 +1622,10 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
> * EXTENT_DEFRAG | EXTENT_CLEAR_META_RESV are handled by the cleanup
> * function.
> *
> - * However, in case of unlock == 0, we still need to unlock the pages
> - * (except @locked_page) to ensure all the pages are unlocked.
> + * However, in case of CFR_KEEP_LOCKED, we still need to unlock the
> + * pages (except @locked_page) to ensure all the pages are unlocked.
> */
> - if (!unlock && orig_start < start) {
> + if ((flags & CFR_KEEP_LOCKED) && orig_start < start) {
> if (!locked_page)
> mapping_set_error(inode->vfs_inode.i_mapping, ret);
> extent_clear_unlock_delalloc(inode, orig_start, start - 1,
> @@ -1836,7 +1831,7 @@ static noinline int run_delalloc_zoned(struct btrfs_inode *inode,
>
> while (start <= end) {
> ret = cow_file_range(inode, locked_page, start, end, page_started,
> - nr_written, 0, &done_offset);
> + nr_written, &done_offset, CFR_KEEP_LOCKED);
> if (ret && ret != -EAGAIN)
> return ret;
>
> @@ -1956,7 +1951,7 @@ static int fallback_to_cow(struct btrfs_inode *inode, struct page *locked_page,
> }
>
> return cow_file_range(inode, locked_page, start, end, page_started,
> - nr_written, 1, NULL);
> + nr_written, NULL, 0);
> }
>
> struct can_nocow_file_extent_args {
> @@ -2433,7 +2428,7 @@ int btrfs_run_delalloc_range(struct btrfs_inode *inode, struct page *locked_page
> page_started, nr_written, wbc);
> else
> ret = cow_file_range(inode, locked_page, start, end,
> - page_started, nr_written, 1, NULL);
> + page_started, nr_written, NULL, 0);
>
> out:
> ASSERT(ret <= 0);
> --
> 2.39.2
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 01/23] btrfs: pass a flags argument to cow_file_range
2023-07-20 11:22 ` David Sterba
@ 2023-07-20 13:25 ` Christoph Hellwig
0 siblings, 0 replies; 5+ messages in thread
From: Christoph Hellwig @ 2023-07-20 13:25 UTC (permalink / raw)
To: David Sterba
Cc: Christoph Hellwig, Chris Mason, Josef Bacik, David Sterba,
Matthew Wilcox, linux-btrfs, linux-fsdevel
On Thu, Jul 20, 2023 at 01:22:36PM +0200, David Sterba wrote:
> On Wed, Jun 28, 2023 at 05:31:22PM +0200, Christoph Hellwig wrote:
> > The int used as bool unlock is not a very good way to describe the
> > behavior, and the next patch will have to add another beahvior modifier.
> > Switch to pass a flag instead, with an inital CFR_KEEP_LOCKED flag that
> > specifies the pages should always be kept locked. This is the inverse
> > of the old unlock argument for the reason that it requires a flag for
> > the exceptional behavior.
>
> Int is the wrong type but I'm not sure that for two flags we should use
> a bit flags. Two bool parameters are IMHO fine and "CFR" does not mean
> anything, it's really only relevant for the function.
CFR stands for cow_file_range. The good news is that with my
(huge) stack of pending pages both flags will eventually go away.
The bad news is that for an intermediate change I actually need a third
one.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 01/23] btrfs: pass a flags argument to cow_file_range
2023-06-28 15:31 ` [PATCH 01/23] btrfs: pass a flags argument to cow_file_range Christoph Hellwig
2023-07-04 8:47 ` Johannes Thumshirn
@ 2023-07-20 11:22 ` David Sterba
2023-07-20 13:25 ` Christoph Hellwig
1 sibling, 1 reply; 5+ messages in thread
From: David Sterba @ 2023-07-20 11:22 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Chris Mason, Josef Bacik, David Sterba, Matthew Wilcox,
linux-btrfs, linux-fsdevel
On Wed, Jun 28, 2023 at 05:31:22PM +0200, Christoph Hellwig wrote:
> The int used as bool unlock is not a very good way to describe the
> behavior, and the next patch will have to add another beahvior modifier.
> Switch to pass a flag instead, with an inital CFR_KEEP_LOCKED flag that
> specifies the pages should always be kept locked. This is the inverse
> of the old unlock argument for the reason that it requires a flag for
> the exceptional behavior.
Int is the wrong type but I'm not sure that for two flags we should use
a bit flags. Two bool parameters are IMHO fine and "CFR" does not mean
anything, it's really only relevant for the function.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 01/23] btrfs: pass a flags argument to cow_file_range
2023-06-28 15:31 ` [PATCH 01/23] btrfs: pass a flags argument to cow_file_range Christoph Hellwig
@ 2023-07-04 8:47 ` Johannes Thumshirn
2023-07-20 11:22 ` David Sterba
1 sibling, 0 replies; 5+ messages in thread
From: Johannes Thumshirn @ 2023-07-04 8:47 UTC (permalink / raw)
To: Christoph Hellwig, Chris Mason, Josef Bacik, David Sterba
Cc: Matthew Wilcox, linux-btrfs, linux-fsdevel
Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 01/23] btrfs: pass a flags argument to cow_file_range
2023-06-28 15:31 btrfs compressed writeback cleanups Christoph Hellwig
@ 2023-06-28 15:31 ` Christoph Hellwig
2023-07-04 8:47 ` Johannes Thumshirn
2023-07-20 11:22 ` David Sterba
0 siblings, 2 replies; 5+ messages in thread
From: Christoph Hellwig @ 2023-06-28 15:31 UTC (permalink / raw)
To: Chris Mason, Josef Bacik, David Sterba
Cc: Matthew Wilcox, linux-btrfs, linux-fsdevel
The int used as bool unlock is not a very good way to describe the
behavior, and the next patch will have to add another beahvior modifier.
Switch to pass a flag instead, with an inital CFR_KEEP_LOCKED flag that
specifies the pages should always be kept locked. This is the inverse
of the old unlock argument for the reason that it requires a flag for
the exceptional behavior.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/btrfs/inode.c | 51 ++++++++++++++++++++++--------------------------
1 file changed, 23 insertions(+), 28 deletions(-)
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index dbbb67293e345c..92a78940991fcb 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -124,11 +124,13 @@ static struct kmem_cache *btrfs_inode_cachep;
static int btrfs_setsize(struct inode *inode, struct iattr *attr);
static int btrfs_truncate(struct btrfs_inode *inode, bool skip_writeback);
+
+#define CFR_KEEP_LOCKED (1 << 0)
static noinline int cow_file_range(struct btrfs_inode *inode,
struct page *locked_page,
u64 start, u64 end, int *page_started,
- unsigned long *nr_written, int unlock,
- u64 *done_offset);
+ unsigned long *nr_written, u64 *done_offset,
+ u32 flags);
static struct extent_map *create_io_em(struct btrfs_inode *inode, u64 start,
u64 len, u64 orig_start, u64 block_start,
u64 block_len, u64 orig_block_len,
@@ -1148,7 +1150,7 @@ static int submit_uncompressed_range(struct btrfs_inode *inode,
* can directly submit them without interruption.
*/
ret = cow_file_range(inode, locked_page, start, end, &page_started,
- &nr_written, 0, NULL);
+ &nr_written, NULL, CFR_KEEP_LOCKED);
/* Inline extent inserted, page gets unlocked and everything is done */
if (page_started)
return 0;
@@ -1362,25 +1364,18 @@ static u64 get_extent_allocation_hint(struct btrfs_inode *inode, u64 start,
* locked_page is the page that writepage had locked already. We use
* it to make sure we don't do extra locks or unlocks.
*
- * *page_started is set to one if we unlock locked_page and do everything
- * required to start IO on it. It may be clean and already done with
- * IO when we return.
- *
- * When unlock == 1, we unlock the pages in successfully allocated regions.
- * When unlock == 0, we leave them locked for writing them out.
+ * When this function fails, it unlocks all pages except @locked_page.
*
- * However, we unlock all the pages except @locked_page in case of failure.
+ * When this function successfully creates an inline extent, it sets page_started
+ * to 1 and unlocks all pages including locked_page and starts I/O on them.
+ * (In reality inline extents are limited to a single page, so locked_page is
+ * the only page handled anyway).
*
- * In summary, page locking state will be as follow:
+ * When this function succeed and creates a normal extent, the page locking
+ * status depends on the passed in flags:
*
- * - page_started == 1 (return value)
- * - All the pages are unlocked. IO is started.
- * - Note that this can happen only on success
- * - unlock == 1
- * - All the pages except @locked_page are unlocked in any case
- * - unlock == 0
- * - On success, all the pages are locked for writing out them
- * - On failure, all the pages except @locked_page are unlocked
+ * - If CFR_KEEP_LOCKED is set, all pages are kept locked.
+ * - Else all pages except for @locked_page are unlocked.
*
* When a failure happens in the second or later iteration of the
* while-loop, the ordered extents created in previous iterations are kept
@@ -1391,8 +1386,8 @@ static u64 get_extent_allocation_hint(struct btrfs_inode *inode, u64 start,
static noinline int cow_file_range(struct btrfs_inode *inode,
struct page *locked_page,
u64 start, u64 end, int *page_started,
- unsigned long *nr_written, int unlock,
- u64 *done_offset)
+ unsigned long *nr_written, u64 *done_offset,
+ u32 flags)
{
struct btrfs_root *root = inode->root;
struct btrfs_fs_info *fs_info = root->fs_info;
@@ -1558,7 +1553,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
* Do set the Ordered (Private2) bit so we know this page was
* properly setup for writepage.
*/
- page_ops = unlock ? PAGE_UNLOCK : 0;
+ page_ops = (flags & CFR_KEEP_LOCKED) ? 0 : PAGE_UNLOCK;
page_ops |= PAGE_SET_ORDERED;
extent_clear_unlock_delalloc(inode, start, start + ram_size - 1,
@@ -1627,10 +1622,10 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
* EXTENT_DEFRAG | EXTENT_CLEAR_META_RESV are handled by the cleanup
* function.
*
- * However, in case of unlock == 0, we still need to unlock the pages
- * (except @locked_page) to ensure all the pages are unlocked.
+ * However, in case of CFR_KEEP_LOCKED, we still need to unlock the
+ * pages (except @locked_page) to ensure all the pages are unlocked.
*/
- if (!unlock && orig_start < start) {
+ if ((flags & CFR_KEEP_LOCKED) && orig_start < start) {
if (!locked_page)
mapping_set_error(inode->vfs_inode.i_mapping, ret);
extent_clear_unlock_delalloc(inode, orig_start, start - 1,
@@ -1836,7 +1831,7 @@ static noinline int run_delalloc_zoned(struct btrfs_inode *inode,
while (start <= end) {
ret = cow_file_range(inode, locked_page, start, end, page_started,
- nr_written, 0, &done_offset);
+ nr_written, &done_offset, CFR_KEEP_LOCKED);
if (ret && ret != -EAGAIN)
return ret;
@@ -1956,7 +1951,7 @@ static int fallback_to_cow(struct btrfs_inode *inode, struct page *locked_page,
}
return cow_file_range(inode, locked_page, start, end, page_started,
- nr_written, 1, NULL);
+ nr_written, NULL, 0);
}
struct can_nocow_file_extent_args {
@@ -2433,7 +2428,7 @@ int btrfs_run_delalloc_range(struct btrfs_inode *inode, struct page *locked_page
page_started, nr_written, wbc);
else
ret = cow_file_range(inode, locked_page, start, end,
- page_started, nr_written, 1, NULL);
+ page_started, nr_written, NULL, 0);
out:
ASSERT(ret <= 0);
--
2.39.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-07-20 13:25 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-29 2:34 [PATCH 01/23] btrfs: pass a flags argument to cow_file_range Naohiro Aota
-- strict thread matches above, loose matches on Subject: below --
2023-06-28 15:31 btrfs compressed writeback cleanups Christoph Hellwig
2023-06-28 15:31 ` [PATCH 01/23] btrfs: pass a flags argument to cow_file_range Christoph Hellwig
2023-07-04 8:47 ` Johannes Thumshirn
2023-07-20 11:22 ` David Sterba
2023-07-20 13:25 ` Christoph Hellwig
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).