* [PATCH 01/10] btrfs: switch uptodate to bool in btrfs_writepage_endio_finish_ordered
2021-07-26 12:15 [PATCH 00/10] Misc small cleanups David Sterba
@ 2021-07-26 12:15 ` David Sterba
2021-07-26 12:23 ` Qu Wenruo
2021-07-26 12:15 ` [PATCH 02/10] btrfs: remove uptodate parameter from btrfs_dec_test_first_ordered_pending David Sterba
` (8 subsequent siblings)
9 siblings, 1 reply; 33+ messages in thread
From: David Sterba @ 2021-07-26 12:15 UTC (permalink / raw)
To: linux-btrfs; +Cc: David Sterba
The uptodate parameter should be bool, change the type.
Signed-off-by: David Sterba <dsterba@suse.com>
---
fs/btrfs/ctree.h | 2 +-
fs/btrfs/extent_io.c | 8 ++++----
fs/btrfs/inode.c | 4 ++--
3 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 4a69aa604ac5..a822404eeaee 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -3195,7 +3195,7 @@ int btrfs_run_delalloc_range(struct btrfs_inode *inode, struct page *locked_page
int btrfs_writepage_cow_fixup(struct page *page, u64 start, u64 end);
void btrfs_writepage_endio_finish_ordered(struct btrfs_inode *inode,
struct page *page, u64 start,
- u64 end, int uptodate);
+ u64 end, bool uptodate);
extern const struct dentry_operations btrfs_dentry_operations;
extern const struct iomap_ops btrfs_dio_iomap_ops;
extern const struct iomap_dio_ops btrfs_dio_ops;
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 1f947e24091a..f7e58c304fc9 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -2779,7 +2779,7 @@ static blk_status_t submit_read_repair(struct inode *inode,
void end_extent_writepage(struct page *page, int err, u64 start, u64 end)
{
struct btrfs_inode *inode;
- int uptodate = (err == 0);
+ const bool uptodate = (err == 0);
int ret = 0;
ASSERT(page && page->mapping);
@@ -3864,7 +3864,7 @@ static noinline_for_stack int __extent_writepage_io(struct btrfs_inode *inode,
if (cur >= i_size) {
btrfs_writepage_endio_finish_ordered(inode, page, cur,
- end, 1);
+ end, true);
break;
}
@@ -3914,7 +3914,7 @@ static noinline_for_stack int __extent_writepage_io(struct btrfs_inode *inode,
nr++;
else
btrfs_writepage_endio_finish_ordered(inode,
- page, cur, cur + iosize - 1, 1);
+ page, cur, cur + iosize - 1, true);
cur += iosize;
continue;
}
@@ -4983,7 +4983,7 @@ int extent_write_locked_range(struct inode *inode, u64 start, u64 end,
ret = __extent_writepage(page, &wbc_writepages, &epd);
else {
btrfs_writepage_endio_finish_ordered(BTRFS_I(inode),
- page, start, start + PAGE_SIZE - 1, 1);
+ page, start, start + PAGE_SIZE - 1, true);
unlock_page(page);
}
put_page(page);
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 6089c5e7763c..43b1393eec67 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -972,7 +972,7 @@ static noinline void submit_compressed_extents(struct async_chunk *async_chunk)
p->mapping = inode->vfs_inode.i_mapping;
btrfs_writepage_endio_finish_ordered(inode, p, start,
- end, 0);
+ end, false);
p->mapping = NULL;
extent_clear_unlock_delalloc(inode, start, end, NULL, 0,
@@ -3170,7 +3170,7 @@ static void finish_ordered_fn(struct btrfs_work *work)
void btrfs_writepage_endio_finish_ordered(struct btrfs_inode *inode,
struct page *page, u64 start,
- u64 end, int uptodate)
+ u64 end, bool uptodate)
{
trace_btrfs_writepage_end_io_hook(inode, start, end, uptodate);
--
2.31.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH 01/10] btrfs: switch uptodate to bool in btrfs_writepage_endio_finish_ordered
2021-07-26 12:15 ` [PATCH 01/10] btrfs: switch uptodate to bool in btrfs_writepage_endio_finish_ordered David Sterba
@ 2021-07-26 12:23 ` Qu Wenruo
0 siblings, 0 replies; 33+ messages in thread
From: Qu Wenruo @ 2021-07-26 12:23 UTC (permalink / raw)
To: David Sterba, linux-btrfs
On 2021/7/26 下午8:15, David Sterba wrote:
> The uptodate parameter should be bool, change the type.
>
> Signed-off-by: David Sterba <dsterba@suse.com>
Reviewed-by: Qu Wenruo <wqu@suse.com>
Thanks,
Qu
> ---
> fs/btrfs/ctree.h | 2 +-
> fs/btrfs/extent_io.c | 8 ++++----
> fs/btrfs/inode.c | 4 ++--
> 3 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 4a69aa604ac5..a822404eeaee 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -3195,7 +3195,7 @@ int btrfs_run_delalloc_range(struct btrfs_inode *inode, struct page *locked_page
> int btrfs_writepage_cow_fixup(struct page *page, u64 start, u64 end);
> void btrfs_writepage_endio_finish_ordered(struct btrfs_inode *inode,
> struct page *page, u64 start,
> - u64 end, int uptodate);
> + u64 end, bool uptodate);
> extern const struct dentry_operations btrfs_dentry_operations;
> extern const struct iomap_ops btrfs_dio_iomap_ops;
> extern const struct iomap_dio_ops btrfs_dio_ops;
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 1f947e24091a..f7e58c304fc9 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -2779,7 +2779,7 @@ static blk_status_t submit_read_repair(struct inode *inode,
> void end_extent_writepage(struct page *page, int err, u64 start, u64 end)
> {
> struct btrfs_inode *inode;
> - int uptodate = (err == 0);
> + const bool uptodate = (err == 0);
> int ret = 0;
>
> ASSERT(page && page->mapping);
> @@ -3864,7 +3864,7 @@ static noinline_for_stack int __extent_writepage_io(struct btrfs_inode *inode,
>
> if (cur >= i_size) {
> btrfs_writepage_endio_finish_ordered(inode, page, cur,
> - end, 1);
> + end, true);
> break;
> }
>
> @@ -3914,7 +3914,7 @@ static noinline_for_stack int __extent_writepage_io(struct btrfs_inode *inode,
> nr++;
> else
> btrfs_writepage_endio_finish_ordered(inode,
> - page, cur, cur + iosize - 1, 1);
> + page, cur, cur + iosize - 1, true);
> cur += iosize;
> continue;
> }
> @@ -4983,7 +4983,7 @@ int extent_write_locked_range(struct inode *inode, u64 start, u64 end,
> ret = __extent_writepage(page, &wbc_writepages, &epd);
> else {
> btrfs_writepage_endio_finish_ordered(BTRFS_I(inode),
> - page, start, start + PAGE_SIZE - 1, 1);
> + page, start, start + PAGE_SIZE - 1, true);
> unlock_page(page);
> }
> put_page(page);
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 6089c5e7763c..43b1393eec67 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -972,7 +972,7 @@ static noinline void submit_compressed_extents(struct async_chunk *async_chunk)
>
> p->mapping = inode->vfs_inode.i_mapping;
> btrfs_writepage_endio_finish_ordered(inode, p, start,
> - end, 0);
> + end, false);
>
> p->mapping = NULL;
> extent_clear_unlock_delalloc(inode, start, end, NULL, 0,
> @@ -3170,7 +3170,7 @@ static void finish_ordered_fn(struct btrfs_work *work)
>
> void btrfs_writepage_endio_finish_ordered(struct btrfs_inode *inode,
> struct page *page, u64 start,
> - u64 end, int uptodate)
> + u64 end, bool uptodate)
> {
> trace_btrfs_writepage_end_io_hook(inode, start, end, uptodate);
>
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 02/10] btrfs: remove uptodate parameter from btrfs_dec_test_first_ordered_pending
2021-07-26 12:15 [PATCH 00/10] Misc small cleanups David Sterba
2021-07-26 12:15 ` [PATCH 01/10] btrfs: switch uptodate to bool in btrfs_writepage_endio_finish_ordered David Sterba
@ 2021-07-26 12:15 ` David Sterba
2021-07-26 12:24 ` Qu Wenruo
2021-07-26 12:15 ` [PATCH 03/10] btrfs: make btrfs_next_leaf static inline David Sterba
` (7 subsequent siblings)
9 siblings, 1 reply; 33+ messages in thread
From: David Sterba @ 2021-07-26 12:15 UTC (permalink / raw)
To: linux-btrfs; +Cc: David Sterba
In commit e65f152e4348 ("btrfs: refactor how we finish ordered extent io
for endio functions") there was last caller not using 1 for the uptodate
parameter. Now there's only one, passing 1, so we can remove it and
simplify the code.
Signed-off-by: David Sterba <dsterba@suse.com>
---
fs/btrfs/inode.c | 2 +-
fs/btrfs/ordered-data.c | 5 +----
fs/btrfs/ordered-data.h | 2 +-
3 files changed, 3 insertions(+), 6 deletions(-)
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 43b1393eec67..ba0bba9f5505 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -8554,7 +8554,7 @@ static void btrfs_invalidatepage(struct page *page, unsigned int offset,
spin_unlock_irq(&inode->ordered_tree.lock);
if (btrfs_dec_test_ordered_pending(inode, &ordered,
- cur, range_end + 1 - cur, 1)) {
+ cur, range_end + 1 - cur)) {
btrfs_finish_ordered_io(ordered);
/*
* The ordered extent has finished, now we're again
diff --git a/fs/btrfs/ordered-data.c b/fs/btrfs/ordered-data.c
index 5c0f8481e25e..edb65abf0393 100644
--- a/fs/btrfs/ordered-data.c
+++ b/fs/btrfs/ordered-data.c
@@ -446,7 +446,6 @@ void btrfs_mark_ordered_io_finished(struct btrfs_inode *inode,
* Will be also used to store the finished ordered extent.
* @file_offset: File offset for the finished IO
* @io_size: Length of the finish IO range
- * @uptodate: If the IO finishes without problem
*
* Return true if the ordered extent is finished in the range, and update
* @cached.
@@ -457,7 +456,7 @@ void btrfs_mark_ordered_io_finished(struct btrfs_inode *inode,
*/
bool btrfs_dec_test_ordered_pending(struct btrfs_inode *inode,
struct btrfs_ordered_extent **cached,
- u64 file_offset, u64 io_size, int uptodate)
+ u64 file_offset, u64 io_size)
{
struct btrfs_ordered_inode_tree *tree = &inode->ordered_tree;
struct rb_node *node;
@@ -486,8 +485,6 @@ bool btrfs_dec_test_ordered_pending(struct btrfs_inode *inode,
entry->bytes_left, io_size);
entry->bytes_left -= io_size;
- if (!uptodate)
- set_bit(BTRFS_ORDERED_IOERR, &entry->flags);
if (entry->bytes_left == 0) {
/*
diff --git a/fs/btrfs/ordered-data.h b/fs/btrfs/ordered-data.h
index b2d88aba8420..4194e960ff61 100644
--- a/fs/btrfs/ordered-data.h
+++ b/fs/btrfs/ordered-data.h
@@ -177,7 +177,7 @@ void btrfs_mark_ordered_io_finished(struct btrfs_inode *inode,
bool uptodate);
bool btrfs_dec_test_ordered_pending(struct btrfs_inode *inode,
struct btrfs_ordered_extent **cached,
- u64 file_offset, u64 io_size, int uptodate);
+ u64 file_offset, u64 io_size);
int btrfs_add_ordered_extent(struct btrfs_inode *inode, u64 file_offset,
u64 disk_bytenr, u64 num_bytes, u64 disk_num_bytes,
int type);
--
2.31.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH 02/10] btrfs: remove uptodate parameter from btrfs_dec_test_first_ordered_pending
2021-07-26 12:15 ` [PATCH 02/10] btrfs: remove uptodate parameter from btrfs_dec_test_first_ordered_pending David Sterba
@ 2021-07-26 12:24 ` Qu Wenruo
0 siblings, 0 replies; 33+ messages in thread
From: Qu Wenruo @ 2021-07-26 12:24 UTC (permalink / raw)
To: David Sterba, linux-btrfs
On 2021/7/26 下午8:15, David Sterba wrote:
> In commit e65f152e4348 ("btrfs: refactor how we finish ordered extent io
> for endio functions") there was last caller not using 1 for the uptodate
> parameter. Now there's only one, passing 1, so we can remove it and
> simplify the code.
I should have noticed that...
>
> Signed-off-by: David Sterba <dsterba@suse.com>
Reviewed-by: Qu Wenruo <wqu@suse.com>
Thanks,
Qu
> ---
> fs/btrfs/inode.c | 2 +-
> fs/btrfs/ordered-data.c | 5 +----
> fs/btrfs/ordered-data.h | 2 +-
> 3 files changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 43b1393eec67..ba0bba9f5505 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -8554,7 +8554,7 @@ static void btrfs_invalidatepage(struct page *page, unsigned int offset,
> spin_unlock_irq(&inode->ordered_tree.lock);
>
> if (btrfs_dec_test_ordered_pending(inode, &ordered,
> - cur, range_end + 1 - cur, 1)) {
> + cur, range_end + 1 - cur)) {
> btrfs_finish_ordered_io(ordered);
> /*
> * The ordered extent has finished, now we're again
> diff --git a/fs/btrfs/ordered-data.c b/fs/btrfs/ordered-data.c
> index 5c0f8481e25e..edb65abf0393 100644
> --- a/fs/btrfs/ordered-data.c
> +++ b/fs/btrfs/ordered-data.c
> @@ -446,7 +446,6 @@ void btrfs_mark_ordered_io_finished(struct btrfs_inode *inode,
> * Will be also used to store the finished ordered extent.
> * @file_offset: File offset for the finished IO
> * @io_size: Length of the finish IO range
> - * @uptodate: If the IO finishes without problem
> *
> * Return true if the ordered extent is finished in the range, and update
> * @cached.
> @@ -457,7 +456,7 @@ void btrfs_mark_ordered_io_finished(struct btrfs_inode *inode,
> */
> bool btrfs_dec_test_ordered_pending(struct btrfs_inode *inode,
> struct btrfs_ordered_extent **cached,
> - u64 file_offset, u64 io_size, int uptodate)
> + u64 file_offset, u64 io_size)
> {
> struct btrfs_ordered_inode_tree *tree = &inode->ordered_tree;
> struct rb_node *node;
> @@ -486,8 +485,6 @@ bool btrfs_dec_test_ordered_pending(struct btrfs_inode *inode,
> entry->bytes_left, io_size);
>
> entry->bytes_left -= io_size;
> - if (!uptodate)
> - set_bit(BTRFS_ORDERED_IOERR, &entry->flags);
>
> if (entry->bytes_left == 0) {
> /*
> diff --git a/fs/btrfs/ordered-data.h b/fs/btrfs/ordered-data.h
> index b2d88aba8420..4194e960ff61 100644
> --- a/fs/btrfs/ordered-data.h
> +++ b/fs/btrfs/ordered-data.h
> @@ -177,7 +177,7 @@ void btrfs_mark_ordered_io_finished(struct btrfs_inode *inode,
> bool uptodate);
> bool btrfs_dec_test_ordered_pending(struct btrfs_inode *inode,
> struct btrfs_ordered_extent **cached,
> - u64 file_offset, u64 io_size, int uptodate);
> + u64 file_offset, u64 io_size);
> int btrfs_add_ordered_extent(struct btrfs_inode *inode, u64 file_offset,
> u64 disk_bytenr, u64 num_bytes, u64 disk_num_bytes,
> int type);
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 03/10] btrfs: make btrfs_next_leaf static inline
2021-07-26 12:15 [PATCH 00/10] Misc small cleanups David Sterba
2021-07-26 12:15 ` [PATCH 01/10] btrfs: switch uptodate to bool in btrfs_writepage_endio_finish_ordered David Sterba
2021-07-26 12:15 ` [PATCH 02/10] btrfs: remove uptodate parameter from btrfs_dec_test_first_ordered_pending David Sterba
@ 2021-07-26 12:15 ` David Sterba
2021-07-26 12:25 ` Qu Wenruo
2021-07-26 12:15 ` [PATCH 04/10] btrfs: tree-checker: use table values for stripe checks David Sterba
` (6 subsequent siblings)
9 siblings, 1 reply; 33+ messages in thread
From: David Sterba @ 2021-07-26 12:15 UTC (permalink / raw)
To: linux-btrfs; +Cc: David Sterba
btrfs_next_leaf is a simple wrapper for btrfs_next_old_leaf so move it
to header to avoid the function call overhead.
Signed-off-by: David Sterba <dsterba@suse.com>
---
fs/btrfs/ctree.c | 10 ----------
fs/btrfs/ctree.h | 13 ++++++++++++-
2 files changed, 12 insertions(+), 11 deletions(-)
diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 63c026495193..99b33a5b33c8 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -4357,16 +4357,6 @@ int btrfs_find_next_key(struct btrfs_root *root, struct btrfs_path *path,
return 1;
}
-/*
- * search the tree again to find a leaf with greater keys
- * returns 0 if it found something or 1 if there are no greater leaves.
- * returns < 0 on io errors.
- */
-int btrfs_next_leaf(struct btrfs_root *root, struct btrfs_path *path)
-{
- return btrfs_next_old_leaf(root, path, 0);
-}
-
int btrfs_next_old_leaf(struct btrfs_root *root, struct btrfs_path *path,
u64 time_seq)
{
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index a822404eeaee..04779e3d3ab5 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -2899,7 +2899,6 @@ static inline int btrfs_insert_empty_item(struct btrfs_trans_handle *trans,
return btrfs_insert_empty_items(trans, root, path, key, &data_size, 1);
}
-int btrfs_next_leaf(struct btrfs_root *root, struct btrfs_path *path);
int btrfs_prev_leaf(struct btrfs_root *root, struct btrfs_path *path);
int btrfs_next_old_leaf(struct btrfs_root *root, struct btrfs_path *path,
u64 time_seq);
@@ -2911,6 +2910,18 @@ static inline int btrfs_next_old_item(struct btrfs_root *root,
return btrfs_next_old_leaf(root, p, time_seq);
return 0;
}
+
+/*
+ * Search the tree again to find a leaf with greater keys.
+ *
+ * Returns 0 if it found something or 1 if there are no greater leaves.
+ * Returns < 0 on error.
+ */
+static inline int btrfs_next_leaf(struct btrfs_root *root, struct btrfs_path *path)
+{
+ return btrfs_next_old_leaf(root, path, 0);
+}
+
static inline int btrfs_next_item(struct btrfs_root *root, struct btrfs_path *p)
{
return btrfs_next_old_item(root, p, 0);
--
2.31.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH 03/10] btrfs: make btrfs_next_leaf static inline
2021-07-26 12:15 ` [PATCH 03/10] btrfs: make btrfs_next_leaf static inline David Sterba
@ 2021-07-26 12:25 ` Qu Wenruo
0 siblings, 0 replies; 33+ messages in thread
From: Qu Wenruo @ 2021-07-26 12:25 UTC (permalink / raw)
To: David Sterba, linux-btrfs
On 2021/7/26 下午8:15, David Sterba wrote:
> btrfs_next_leaf is a simple wrapper for btrfs_next_old_leaf so move it
> to header to avoid the function call overhead.
>
> Signed-off-by: David Sterba <dsterba@suse.com>
Reviewed-by: Qu Wenruo <wqu@suse.com>
Thanks,
Qu
> ---
> fs/btrfs/ctree.c | 10 ----------
> fs/btrfs/ctree.h | 13 ++++++++++++-
> 2 files changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index 63c026495193..99b33a5b33c8 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -4357,16 +4357,6 @@ int btrfs_find_next_key(struct btrfs_root *root, struct btrfs_path *path,
> return 1;
> }
>
> -/*
> - * search the tree again to find a leaf with greater keys
> - * returns 0 if it found something or 1 if there are no greater leaves.
> - * returns < 0 on io errors.
> - */
> -int btrfs_next_leaf(struct btrfs_root *root, struct btrfs_path *path)
> -{
> - return btrfs_next_old_leaf(root, path, 0);
> -}
> -
> int btrfs_next_old_leaf(struct btrfs_root *root, struct btrfs_path *path,
> u64 time_seq)
> {
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index a822404eeaee..04779e3d3ab5 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -2899,7 +2899,6 @@ static inline int btrfs_insert_empty_item(struct btrfs_trans_handle *trans,
> return btrfs_insert_empty_items(trans, root, path, key, &data_size, 1);
> }
>
> -int btrfs_next_leaf(struct btrfs_root *root, struct btrfs_path *path);
> int btrfs_prev_leaf(struct btrfs_root *root, struct btrfs_path *path);
> int btrfs_next_old_leaf(struct btrfs_root *root, struct btrfs_path *path,
> u64 time_seq);
> @@ -2911,6 +2910,18 @@ static inline int btrfs_next_old_item(struct btrfs_root *root,
> return btrfs_next_old_leaf(root, p, time_seq);
> return 0;
> }
> +
> +/*
> + * Search the tree again to find a leaf with greater keys.
> + *
> + * Returns 0 if it found something or 1 if there are no greater leaves.
> + * Returns < 0 on error.
> + */
> +static inline int btrfs_next_leaf(struct btrfs_root *root, struct btrfs_path *path)
> +{
> + return btrfs_next_old_leaf(root, path, 0);
> +}
> +
> static inline int btrfs_next_item(struct btrfs_root *root, struct btrfs_path *p)
> {
> return btrfs_next_old_item(root, p, 0);
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 04/10] btrfs: tree-checker: use table values for stripe checks
2021-07-26 12:15 [PATCH 00/10] Misc small cleanups David Sterba
` (2 preceding siblings ...)
2021-07-26 12:15 ` [PATCH 03/10] btrfs: make btrfs_next_leaf static inline David Sterba
@ 2021-07-26 12:15 ` David Sterba
2021-07-26 12:29 ` Qu Wenruo
2021-07-26 12:15 ` [PATCH 05/10] btrfs: tree-checker: add missing stripe checks for raid1c3/4 profiles David Sterba
` (5 subsequent siblings)
9 siblings, 1 reply; 33+ messages in thread
From: David Sterba @ 2021-07-26 12:15 UTC (permalink / raw)
To: linux-btrfs; +Cc: David Sterba
There are hardcoded values in several checks regarding chunks and stripe
constraints. We have that defined in the raid table and ought to use it.
Signed-off-by: David Sterba <dsterba@suse.com>
---
fs/btrfs/tree-checker.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
index a8b2e0d2c025..ac9416cb4496 100644
--- a/fs/btrfs/tree-checker.c
+++ b/fs/btrfs/tree-checker.c
@@ -873,13 +873,18 @@ int btrfs_check_chunk_valid(struct extent_buffer *leaf,
}
}
- if (unlikely((type & BTRFS_BLOCK_GROUP_RAID10 && sub_stripes != 2) ||
- (type & BTRFS_BLOCK_GROUP_RAID1 && num_stripes != 2) ||
- (type & BTRFS_BLOCK_GROUP_RAID5 && num_stripes < 2) ||
- (type & BTRFS_BLOCK_GROUP_RAID6 && num_stripes < 3) ||
- (type & BTRFS_BLOCK_GROUP_DUP && num_stripes != 2) ||
+ if (unlikely((type & BTRFS_BLOCK_GROUP_RAID10 &&
+ sub_stripes != btrfs_raid_array[BTRFS_RAID_RAID10].sub_stripes) ||
+ (type & BTRFS_BLOCK_GROUP_RAID1 &&
+ num_stripes != btrfs_raid_array[BTRFS_RAID_RAID1].devs_min) ||
+ (type & BTRFS_BLOCK_GROUP_RAID5 &&
+ num_stripes < btrfs_raid_array[BTRFS_RAID_RAID5].devs_min) ||
+ (type & BTRFS_BLOCK_GROUP_RAID6 &&
+ num_stripes < btrfs_raid_array[BTRFS_RAID_RAID6].devs_min) ||
+ (type & BTRFS_BLOCK_GROUP_DUP &&
+ num_stripes != btrfs_raid_array[BTRFS_RAID_DUP].dev_stripes) ||
((type & BTRFS_BLOCK_GROUP_PROFILE_MASK) == 0 &&
- num_stripes != 1))) {
+ num_stripes != btrfs_raid_array[BTRFS_RAID_SINGLE].dev_stripes))) {
chunk_err(leaf, chunk, logical,
"invalid num_stripes:sub_stripes %u:%u for profile %llu",
num_stripes, sub_stripes,
--
2.31.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH 04/10] btrfs: tree-checker: use table values for stripe checks
2021-07-26 12:15 ` [PATCH 04/10] btrfs: tree-checker: use table values for stripe checks David Sterba
@ 2021-07-26 12:29 ` Qu Wenruo
2021-07-26 14:47 ` David Sterba
0 siblings, 1 reply; 33+ messages in thread
From: Qu Wenruo @ 2021-07-26 12:29 UTC (permalink / raw)
To: David Sterba, linux-btrfs
On 2021/7/26 下午8:15, David Sterba wrote:
> There are hardcoded values in several checks regarding chunks and stripe
> constraints. We have that defined in the raid table and ought to use it.
>
> Signed-off-by: David Sterba <dsterba@suse.com>
Reviewed-by: Qu Wenruo <wqu@suse.com>
But one weird off-topics inlined below.
> ---
> fs/btrfs/tree-checker.c | 17 +++++++++++------
> 1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
> index a8b2e0d2c025..ac9416cb4496 100644
> --- a/fs/btrfs/tree-checker.c
> +++ b/fs/btrfs/tree-checker.c
> @@ -873,13 +873,18 @@ int btrfs_check_chunk_valid(struct extent_buffer *leaf,
> }
> }
>
> - if (unlikely((type & BTRFS_BLOCK_GROUP_RAID10 && sub_stripes != 2) ||
> - (type & BTRFS_BLOCK_GROUP_RAID1 && num_stripes != 2) ||
> - (type & BTRFS_BLOCK_GROUP_RAID5 && num_stripes < 2) ||
> - (type & BTRFS_BLOCK_GROUP_RAID6 && num_stripes < 3) ||
> - (type & BTRFS_BLOCK_GROUP_DUP && num_stripes != 2) ||
> + if (unlikely((type & BTRFS_BLOCK_GROUP_RAID10 &&
> + sub_stripes != btrfs_raid_array[BTRFS_RAID_RAID10].sub_stripes) ||
> + (type & BTRFS_BLOCK_GROUP_RAID1 &&
> + num_stripes != btrfs_raid_array[BTRFS_RAID_RAID1].devs_min) ||
We're adding support for single device RAID0, but there won't be
anything called single device RAID1 at all, right?
Thanks,
Qu
> + (type & BTRFS_BLOCK_GROUP_RAID5 &&
> + num_stripes < btrfs_raid_array[BTRFS_RAID_RAID5].devs_min) ||
> + (type & BTRFS_BLOCK_GROUP_RAID6 &&
> + num_stripes < btrfs_raid_array[BTRFS_RAID_RAID6].devs_min) ||
> + (type & BTRFS_BLOCK_GROUP_DUP &&
> + num_stripes != btrfs_raid_array[BTRFS_RAID_DUP].dev_stripes) ||
> ((type & BTRFS_BLOCK_GROUP_PROFILE_MASK) == 0 &&
> - num_stripes != 1))) {
> + num_stripes != btrfs_raid_array[BTRFS_RAID_SINGLE].dev_stripes))) {
> chunk_err(leaf, chunk, logical,
> "invalid num_stripes:sub_stripes %u:%u for profile %llu",
> num_stripes, sub_stripes,
>
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH 04/10] btrfs: tree-checker: use table values for stripe checks
2021-07-26 12:29 ` Qu Wenruo
@ 2021-07-26 14:47 ` David Sterba
0 siblings, 0 replies; 33+ messages in thread
From: David Sterba @ 2021-07-26 14:47 UTC (permalink / raw)
To: Qu Wenruo; +Cc: David Sterba, linux-btrfs
On Mon, Jul 26, 2021 at 08:29:25PM +0800, Qu Wenruo wrote:
>
>
> On 2021/7/26 下午8:15, David Sterba wrote:
> > There are hardcoded values in several checks regarding chunks and stripe
> > constraints. We have that defined in the raid table and ought to use it.
> >
> > Signed-off-by: David Sterba <dsterba@suse.com>
>
> Reviewed-by: Qu Wenruo <wqu@suse.com>
>
> But one weird off-topics inlined below.
>
> > ---
> > fs/btrfs/tree-checker.c | 17 +++++++++++------
> > 1 file changed, 11 insertions(+), 6 deletions(-)
> >
> > diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
> > index a8b2e0d2c025..ac9416cb4496 100644
> > --- a/fs/btrfs/tree-checker.c
> > +++ b/fs/btrfs/tree-checker.c
> > @@ -873,13 +873,18 @@ int btrfs_check_chunk_valid(struct extent_buffer *leaf,
> > }
> > }
> >
> > - if (unlikely((type & BTRFS_BLOCK_GROUP_RAID10 && sub_stripes != 2) ||
> > - (type & BTRFS_BLOCK_GROUP_RAID1 && num_stripes != 2) ||
> > - (type & BTRFS_BLOCK_GROUP_RAID5 && num_stripes < 2) ||
> > - (type & BTRFS_BLOCK_GROUP_RAID6 && num_stripes < 3) ||
> > - (type & BTRFS_BLOCK_GROUP_DUP && num_stripes != 2) ||
> > + if (unlikely((type & BTRFS_BLOCK_GROUP_RAID10 &&
> > + sub_stripes != btrfs_raid_array[BTRFS_RAID_RAID10].sub_stripes) ||
> > + (type & BTRFS_BLOCK_GROUP_RAID1 &&
> > + num_stripes != btrfs_raid_array[BTRFS_RAID_RAID1].devs_min) ||
>
> We're adding support for single device RAID0, but there won't be
> anything called single device RAID1 at all, right?
Raid 1 with one device can only work in degraded mode so that's
different, it still expects at least 2 devices.
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 05/10] btrfs: tree-checker: add missing stripe checks for raid1c3/4 profiles
2021-07-26 12:15 [PATCH 00/10] Misc small cleanups David Sterba
` (3 preceding siblings ...)
2021-07-26 12:15 ` [PATCH 04/10] btrfs: tree-checker: use table values for stripe checks David Sterba
@ 2021-07-26 12:15 ` David Sterba
2021-07-26 12:29 ` Qu Wenruo
2021-07-26 12:15 ` [PATCH 06/10] btrfs: uninline btrfs_bg_flags_to_raid_index David Sterba
` (4 subsequent siblings)
9 siblings, 1 reply; 33+ messages in thread
From: David Sterba @ 2021-07-26 12:15 UTC (permalink / raw)
To: linux-btrfs; +Cc: David Sterba
The stripe checks for raid1c3/raid1c4 are missing in the sequence in
btrfs_check_chunk_valid.
Signed-off-by: David Sterba <dsterba@suse.com>
---
fs/btrfs/tree-checker.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
index ac9416cb4496..7ba94b683ee3 100644
--- a/fs/btrfs/tree-checker.c
+++ b/fs/btrfs/tree-checker.c
@@ -877,6 +877,10 @@ int btrfs_check_chunk_valid(struct extent_buffer *leaf,
sub_stripes != btrfs_raid_array[BTRFS_RAID_RAID10].sub_stripes) ||
(type & BTRFS_BLOCK_GROUP_RAID1 &&
num_stripes != btrfs_raid_array[BTRFS_RAID_RAID1].devs_min) ||
+ (type & BTRFS_BLOCK_GROUP_RAID1C3 &&
+ num_stripes != btrfs_raid_array[BTRFS_RAID_RAID1C3].devs_min) ||
+ (type & BTRFS_BLOCK_GROUP_RAID1C4 &&
+ num_stripes != btrfs_raid_array[BTRFS_RAID_RAID1C4].devs_min) ||
(type & BTRFS_BLOCK_GROUP_RAID5 &&
num_stripes < btrfs_raid_array[BTRFS_RAID_RAID5].devs_min) ||
(type & BTRFS_BLOCK_GROUP_RAID6 &&
--
2.31.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH 05/10] btrfs: tree-checker: add missing stripe checks for raid1c3/4 profiles
2021-07-26 12:15 ` [PATCH 05/10] btrfs: tree-checker: add missing stripe checks for raid1c3/4 profiles David Sterba
@ 2021-07-26 12:29 ` Qu Wenruo
0 siblings, 0 replies; 33+ messages in thread
From: Qu Wenruo @ 2021-07-26 12:29 UTC (permalink / raw)
To: David Sterba, linux-btrfs
On 2021/7/26 下午8:15, David Sterba wrote:
> The stripe checks for raid1c3/raid1c4 are missing in the sequence in
> btrfs_check_chunk_valid.
>
> Signed-off-by: David Sterba <dsterba@suse.com>
Reviewed-by: Qu Wenruo <wqu@suse.com>
Thanks,
Qu
> ---
> fs/btrfs/tree-checker.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
> index ac9416cb4496..7ba94b683ee3 100644
> --- a/fs/btrfs/tree-checker.c
> +++ b/fs/btrfs/tree-checker.c
> @@ -877,6 +877,10 @@ int btrfs_check_chunk_valid(struct extent_buffer *leaf,
> sub_stripes != btrfs_raid_array[BTRFS_RAID_RAID10].sub_stripes) ||
> (type & BTRFS_BLOCK_GROUP_RAID1 &&
> num_stripes != btrfs_raid_array[BTRFS_RAID_RAID1].devs_min) ||
> + (type & BTRFS_BLOCK_GROUP_RAID1C3 &&
> + num_stripes != btrfs_raid_array[BTRFS_RAID_RAID1C3].devs_min) ||
> + (type & BTRFS_BLOCK_GROUP_RAID1C4 &&
> + num_stripes != btrfs_raid_array[BTRFS_RAID_RAID1C4].devs_min) ||
> (type & BTRFS_BLOCK_GROUP_RAID5 &&
> num_stripes < btrfs_raid_array[BTRFS_RAID_RAID5].devs_min) ||
> (type & BTRFS_BLOCK_GROUP_RAID6 &&
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 06/10] btrfs: uninline btrfs_bg_flags_to_raid_index
2021-07-26 12:15 [PATCH 00/10] Misc small cleanups David Sterba
` (4 preceding siblings ...)
2021-07-26 12:15 ` [PATCH 05/10] btrfs: tree-checker: add missing stripe checks for raid1c3/4 profiles David Sterba
@ 2021-07-26 12:15 ` David Sterba
2021-07-26 12:34 ` Qu Wenruo
2021-07-26 12:15 ` [PATCH 07/10] btrfs: merge alloc_device helpers David Sterba
` (3 subsequent siblings)
9 siblings, 1 reply; 33+ messages in thread
From: David Sterba @ 2021-07-26 12:15 UTC (permalink / raw)
To: linux-btrfs; +Cc: David Sterba
The helper does a simple translation from block group flags to index to
the btrfs_raid_array table. There's no apparent reason to inline the
function, the translation happens usually once per function and is not
called in a loop.
Making it a proper function saves quite some binary code (x86_64,
release config):
text data bss dec hex filename
1164011 19253 14912 1198176 124860 pre/btrfs.ko
1161559 19253 14912 1195724 123ecc post/btrfs.ko
DELTA: -2451
Also add the const attribute as there are no side effects, this could
help compiler to optimize a few things without the function body.
Signed-off-by: David Sterba <dsterba@suse.com>
---
fs/btrfs/volumes.c | 26 ++++++++++++++++++++++++++
fs/btrfs/volumes.h | 27 +--------------------------
2 files changed, 27 insertions(+), 26 deletions(-)
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 86846d6e58d0..19feb64586fc 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -153,6 +153,32 @@ const struct btrfs_raid_attr btrfs_raid_array[BTRFS_NR_RAID_TYPES] = {
},
};
+/*
+ * Convert block group flags (BTRFS_BLOCK_GROUP_*) to btrfs_raid_types, which
+ * can be used as index to access btrfs_raid_array[].
+ */
+enum btrfs_raid_types __attribute_const__ btrfs_bg_flags_to_raid_index(u64 flags)
+{
+ if (flags & BTRFS_BLOCK_GROUP_RAID10)
+ return BTRFS_RAID_RAID10;
+ else if (flags & BTRFS_BLOCK_GROUP_RAID1)
+ return BTRFS_RAID_RAID1;
+ else if (flags & BTRFS_BLOCK_GROUP_RAID1C3)
+ return BTRFS_RAID_RAID1C3;
+ else if (flags & BTRFS_BLOCK_GROUP_RAID1C4)
+ return BTRFS_RAID_RAID1C4;
+ else if (flags & BTRFS_BLOCK_GROUP_DUP)
+ return BTRFS_RAID_DUP;
+ else if (flags & BTRFS_BLOCK_GROUP_RAID0)
+ return BTRFS_RAID_RAID0;
+ else if (flags & BTRFS_BLOCK_GROUP_RAID5)
+ return BTRFS_RAID_RAID5;
+ else if (flags & BTRFS_BLOCK_GROUP_RAID6)
+ return BTRFS_RAID_RAID6;
+
+ return BTRFS_RAID_SINGLE; /* BTRFS_BLOCK_GROUP_SINGLE */
+}
+
const char *btrfs_bg_type_to_raid_name(u64 flags)
{
const int index = btrfs_bg_flags_to_raid_index(flags);
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 70c749eee3ad..b082250b42e0 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -566,32 +566,6 @@ static inline void btrfs_dev_stat_set(struct btrfs_device *dev,
atomic_inc(&dev->dev_stats_ccnt);
}
-/*
- * Convert block group flags (BTRFS_BLOCK_GROUP_*) to btrfs_raid_types, which
- * can be used as index to access btrfs_raid_array[].
- */
-static inline enum btrfs_raid_types btrfs_bg_flags_to_raid_index(u64 flags)
-{
- if (flags & BTRFS_BLOCK_GROUP_RAID10)
- return BTRFS_RAID_RAID10;
- else if (flags & BTRFS_BLOCK_GROUP_RAID1)
- return BTRFS_RAID_RAID1;
- else if (flags & BTRFS_BLOCK_GROUP_RAID1C3)
- return BTRFS_RAID_RAID1C3;
- else if (flags & BTRFS_BLOCK_GROUP_RAID1C4)
- return BTRFS_RAID_RAID1C4;
- else if (flags & BTRFS_BLOCK_GROUP_DUP)
- return BTRFS_RAID_DUP;
- else if (flags & BTRFS_BLOCK_GROUP_RAID0)
- return BTRFS_RAID_RAID0;
- else if (flags & BTRFS_BLOCK_GROUP_RAID5)
- return BTRFS_RAID_RAID5;
- else if (flags & BTRFS_BLOCK_GROUP_RAID6)
- return BTRFS_RAID_RAID6;
-
- return BTRFS_RAID_SINGLE; /* BTRFS_BLOCK_GROUP_SINGLE */
-}
-
void btrfs_commit_device_sizes(struct btrfs_transaction *trans);
struct list_head * __attribute_const__ btrfs_get_fs_uuids(void);
@@ -601,6 +575,7 @@ void btrfs_scratch_superblocks(struct btrfs_fs_info *fs_info,
struct block_device *bdev,
const char *device_path);
+enum btrfs_raid_types __attribute_const__ btrfs_bg_flags_to_raid_index(u64 flags);
int btrfs_bg_type_to_factor(u64 flags);
const char *btrfs_bg_type_to_raid_name(u64 flags);
int btrfs_verify_dev_extents(struct btrfs_fs_info *fs_info);
--
2.31.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH 06/10] btrfs: uninline btrfs_bg_flags_to_raid_index
2021-07-26 12:15 ` [PATCH 06/10] btrfs: uninline btrfs_bg_flags_to_raid_index David Sterba
@ 2021-07-26 12:34 ` Qu Wenruo
2021-07-26 15:06 ` David Sterba
0 siblings, 1 reply; 33+ messages in thread
From: Qu Wenruo @ 2021-07-26 12:34 UTC (permalink / raw)
To: David Sterba, linux-btrfs
On 2021/7/26 下午8:15, David Sterba wrote:
> The helper does a simple translation from block group flags to index to
> the btrfs_raid_array table. There's no apparent reason to inline the
> function, the translation happens usually once per function and is not
> called in a loop.
>
> Making it a proper function saves quite some binary code (x86_64,
> release config):
>
> text data bss dec hex filename
> 1164011 19253 14912 1198176 124860 pre/btrfs.ko
> 1161559 19253 14912 1195724 123ecc post/btrfs.ko
>
> DELTA: -2451
My memory says there used to be some option to allow the compiler to
uninline some functions, but I can't find it in the latest kernel.
It looks to me that this should really be something dependent on kernel
config/compiler optimization.
E.g. to allow -Os optimization to uninline such functions.
Thanks,
Qu
>
> Also add the const attribute as there are no side effects, this could
> help compiler to optimize a few things without the function body.
>
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
> fs/btrfs/volumes.c | 26 ++++++++++++++++++++++++++
> fs/btrfs/volumes.h | 27 +--------------------------
> 2 files changed, 27 insertions(+), 26 deletions(-)
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 86846d6e58d0..19feb64586fc 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -153,6 +153,32 @@ const struct btrfs_raid_attr btrfs_raid_array[BTRFS_NR_RAID_TYPES] = {
> },
> };
>
> +/*
> + * Convert block group flags (BTRFS_BLOCK_GROUP_*) to btrfs_raid_types, which
> + * can be used as index to access btrfs_raid_array[].
> + */
> +enum btrfs_raid_types __attribute_const__ btrfs_bg_flags_to_raid_index(u64 flags)
> +{
> + if (flags & BTRFS_BLOCK_GROUP_RAID10)
> + return BTRFS_RAID_RAID10;
> + else if (flags & BTRFS_BLOCK_GROUP_RAID1)
> + return BTRFS_RAID_RAID1;
> + else if (flags & BTRFS_BLOCK_GROUP_RAID1C3)
> + return BTRFS_RAID_RAID1C3;
> + else if (flags & BTRFS_BLOCK_GROUP_RAID1C4)
> + return BTRFS_RAID_RAID1C4;
> + else if (flags & BTRFS_BLOCK_GROUP_DUP)
> + return BTRFS_RAID_DUP;
> + else if (flags & BTRFS_BLOCK_GROUP_RAID0)
> + return BTRFS_RAID_RAID0;
> + else if (flags & BTRFS_BLOCK_GROUP_RAID5)
> + return BTRFS_RAID_RAID5;
> + else if (flags & BTRFS_BLOCK_GROUP_RAID6)
> + return BTRFS_RAID_RAID6;
> +
> + return BTRFS_RAID_SINGLE; /* BTRFS_BLOCK_GROUP_SINGLE */
> +}
> +
> const char *btrfs_bg_type_to_raid_name(u64 flags)
> {
> const int index = btrfs_bg_flags_to_raid_index(flags);
> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
> index 70c749eee3ad..b082250b42e0 100644
> --- a/fs/btrfs/volumes.h
> +++ b/fs/btrfs/volumes.h
> @@ -566,32 +566,6 @@ static inline void btrfs_dev_stat_set(struct btrfs_device *dev,
> atomic_inc(&dev->dev_stats_ccnt);
> }
>
> -/*
> - * Convert block group flags (BTRFS_BLOCK_GROUP_*) to btrfs_raid_types, which
> - * can be used as index to access btrfs_raid_array[].
> - */
> -static inline enum btrfs_raid_types btrfs_bg_flags_to_raid_index(u64 flags)
> -{
> - if (flags & BTRFS_BLOCK_GROUP_RAID10)
> - return BTRFS_RAID_RAID10;
> - else if (flags & BTRFS_BLOCK_GROUP_RAID1)
> - return BTRFS_RAID_RAID1;
> - else if (flags & BTRFS_BLOCK_GROUP_RAID1C3)
> - return BTRFS_RAID_RAID1C3;
> - else if (flags & BTRFS_BLOCK_GROUP_RAID1C4)
> - return BTRFS_RAID_RAID1C4;
> - else if (flags & BTRFS_BLOCK_GROUP_DUP)
> - return BTRFS_RAID_DUP;
> - else if (flags & BTRFS_BLOCK_GROUP_RAID0)
> - return BTRFS_RAID_RAID0;
> - else if (flags & BTRFS_BLOCK_GROUP_RAID5)
> - return BTRFS_RAID_RAID5;
> - else if (flags & BTRFS_BLOCK_GROUP_RAID6)
> - return BTRFS_RAID_RAID6;
> -
> - return BTRFS_RAID_SINGLE; /* BTRFS_BLOCK_GROUP_SINGLE */
> -}
> -
> void btrfs_commit_device_sizes(struct btrfs_transaction *trans);
>
> struct list_head * __attribute_const__ btrfs_get_fs_uuids(void);
> @@ -601,6 +575,7 @@ void btrfs_scratch_superblocks(struct btrfs_fs_info *fs_info,
> struct block_device *bdev,
> const char *device_path);
>
> +enum btrfs_raid_types __attribute_const__ btrfs_bg_flags_to_raid_index(u64 flags);
> int btrfs_bg_type_to_factor(u64 flags);
> const char *btrfs_bg_type_to_raid_name(u64 flags);
> int btrfs_verify_dev_extents(struct btrfs_fs_info *fs_info);
>
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH 06/10] btrfs: uninline btrfs_bg_flags_to_raid_index
2021-07-26 12:34 ` Qu Wenruo
@ 2021-07-26 15:06 ` David Sterba
0 siblings, 0 replies; 33+ messages in thread
From: David Sterba @ 2021-07-26 15:06 UTC (permalink / raw)
To: Qu Wenruo; +Cc: David Sterba, linux-btrfs
On Mon, Jul 26, 2021 at 08:34:20PM +0800, Qu Wenruo wrote:
>
>
> On 2021/7/26 下午8:15, David Sterba wrote:
> > The helper does a simple translation from block group flags to index to
> > the btrfs_raid_array table. There's no apparent reason to inline the
> > function, the translation happens usually once per function and is not
> > called in a loop.
> >
> > Making it a proper function saves quite some binary code (x86_64,
> > release config):
> >
> > text data bss dec hex filename
> > 1164011 19253 14912 1198176 124860 pre/btrfs.ko
> > 1161559 19253 14912 1195724 123ecc post/btrfs.ko
> >
> > DELTA: -2451
>
> My memory says there used to be some option to allow the compiler to
> uninline some functions, but I can't find it in the latest kernel.
Inlining is the compiler magic, the inline annotations are taken as
hints (that's what compiler people say) but they mostly work as written
in the source.
> It looks to me that this should really be something dependent on kernel
> config/compiler optimization.
Leaving the optimizations to the compiler is a good idea in general,
recompiling the same sources on a newer CPU target can lead to better
code compared to some hand-crafted code that may prevent the
optimizations. The one I uninlined here was an obvious "too big for
inline" and it was not a hot path. I'd rather remove such obvious cases
as we see them, the size and scope of the change is measurable.
> E.g. to allow -Os optimization to uninline such functions.
IIRC -Os is not recommended for kernel as it could generate worse code.
Also I don't think it's not a common default option so we'd have to live
with the worse code for most builds.
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 07/10] btrfs: merge alloc_device helpers
2021-07-26 12:15 [PATCH 00/10] Misc small cleanups David Sterba
` (5 preceding siblings ...)
2021-07-26 12:15 ` [PATCH 06/10] btrfs: uninline btrfs_bg_flags_to_raid_index David Sterba
@ 2021-07-26 12:15 ` David Sterba
2021-07-26 12:35 ` Qu Wenruo
2021-07-26 12:15 ` [PATCH 08/10] btrfs: simplify data stripe calculation helpers David Sterba
` (2 subsequent siblings)
9 siblings, 1 reply; 33+ messages in thread
From: David Sterba @ 2021-07-26 12:15 UTC (permalink / raw)
To: linux-btrfs; +Cc: David Sterba
The device allocation is split to two functions, but one just calls the
other and they're very far in the file. Merge them together.
Signed-off-by: David Sterba <dsterba@suse.com>
---
fs/btrfs/volumes.c | 66 ++++++++++++++++++----------------------------
1 file changed, 25 insertions(+), 41 deletions(-)
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 19feb64586fc..d98e29556d79 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -430,44 +430,6 @@ void __exit btrfs_cleanup_fs_uuids(void)
}
}
-/*
- * Returns a pointer to a new btrfs_device on success; ERR_PTR() on error.
- * Returned struct is not linked onto any lists and must be destroyed using
- * btrfs_free_device.
- */
-static struct btrfs_device *__alloc_device(struct btrfs_fs_info *fs_info)
-{
- struct btrfs_device *dev;
-
- dev = kzalloc(sizeof(*dev), GFP_KERNEL);
- if (!dev)
- return ERR_PTR(-ENOMEM);
-
- /*
- * Preallocate a bio that's always going to be used for flushing device
- * barriers and matches the device lifespan
- */
- dev->flush_bio = bio_kmalloc(GFP_KERNEL, 0);
- if (!dev->flush_bio) {
- kfree(dev);
- return ERR_PTR(-ENOMEM);
- }
-
- INIT_LIST_HEAD(&dev->dev_list);
- INIT_LIST_HEAD(&dev->dev_alloc_list);
- INIT_LIST_HEAD(&dev->post_commit_list);
-
- atomic_set(&dev->reada_in_flight, 0);
- atomic_set(&dev->dev_stats_ccnt, 0);
- btrfs_device_data_ordered_init(dev);
- INIT_RADIX_TREE(&dev->reada_zones, GFP_NOFS & ~__GFP_DIRECT_RECLAIM);
- INIT_RADIX_TREE(&dev->reada_extents, GFP_NOFS & ~__GFP_DIRECT_RECLAIM);
- extent_io_tree_init(fs_info, &dev->alloc_state,
- IO_TREE_DEVICE_ALLOC_STATE, NULL);
-
- return dev;
-}
-
static noinline struct btrfs_fs_devices *find_fsid(
const u8 *fsid, const u8 *metadata_fsid)
{
@@ -6856,9 +6818,31 @@ struct btrfs_device *btrfs_alloc_device(struct btrfs_fs_info *fs_info,
if (WARN_ON(!devid && !fs_info))
return ERR_PTR(-EINVAL);
- dev = __alloc_device(fs_info);
- if (IS_ERR(dev))
- return dev;
+ dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+ if (!dev)
+ return ERR_PTR(-ENOMEM);
+
+ /*
+ * Preallocate a bio that's always going to be used for flushing device
+ * barriers and matches the device lifespan
+ */
+ dev->flush_bio = bio_kmalloc(GFP_KERNEL, 0);
+ if (!dev->flush_bio) {
+ kfree(dev);
+ return ERR_PTR(-ENOMEM);
+ }
+
+ INIT_LIST_HEAD(&dev->dev_list);
+ INIT_LIST_HEAD(&dev->dev_alloc_list);
+ INIT_LIST_HEAD(&dev->post_commit_list);
+
+ atomic_set(&dev->reada_in_flight, 0);
+ atomic_set(&dev->dev_stats_ccnt, 0);
+ btrfs_device_data_ordered_init(dev);
+ INIT_RADIX_TREE(&dev->reada_zones, GFP_NOFS & ~__GFP_DIRECT_RECLAIM);
+ INIT_RADIX_TREE(&dev->reada_extents, GFP_NOFS & ~__GFP_DIRECT_RECLAIM);
+ extent_io_tree_init(fs_info, &dev->alloc_state,
+ IO_TREE_DEVICE_ALLOC_STATE, NULL);
if (devid)
tmp = *devid;
--
2.31.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH 07/10] btrfs: merge alloc_device helpers
2021-07-26 12:15 ` [PATCH 07/10] btrfs: merge alloc_device helpers David Sterba
@ 2021-07-26 12:35 ` Qu Wenruo
0 siblings, 0 replies; 33+ messages in thread
From: Qu Wenruo @ 2021-07-26 12:35 UTC (permalink / raw)
To: David Sterba, linux-btrfs
On 2021/7/26 下午8:15, David Sterba wrote:
> The device allocation is split to two functions, but one just calls the
> other and they're very far in the file. Merge them together.
>
> Signed-off-by: David Sterba <dsterba@suse.com>
Reviewed-by: Qu Wenruo <wqu@suse.com>
Thanks,
Qu
> ---
> fs/btrfs/volumes.c | 66 ++++++++++++++++++----------------------------
> 1 file changed, 25 insertions(+), 41 deletions(-)
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 19feb64586fc..d98e29556d79 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -430,44 +430,6 @@ void __exit btrfs_cleanup_fs_uuids(void)
> }
> }
>
> -/*
> - * Returns a pointer to a new btrfs_device on success; ERR_PTR() on error.
> - * Returned struct is not linked onto any lists and must be destroyed using
> - * btrfs_free_device.
> - */
> -static struct btrfs_device *__alloc_device(struct btrfs_fs_info *fs_info)
> -{
> - struct btrfs_device *dev;
> -
> - dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> - if (!dev)
> - return ERR_PTR(-ENOMEM);
> -
> - /*
> - * Preallocate a bio that's always going to be used for flushing device
> - * barriers and matches the device lifespan
> - */
> - dev->flush_bio = bio_kmalloc(GFP_KERNEL, 0);
> - if (!dev->flush_bio) {
> - kfree(dev);
> - return ERR_PTR(-ENOMEM);
> - }
> -
> - INIT_LIST_HEAD(&dev->dev_list);
> - INIT_LIST_HEAD(&dev->dev_alloc_list);
> - INIT_LIST_HEAD(&dev->post_commit_list);
> -
> - atomic_set(&dev->reada_in_flight, 0);
> - atomic_set(&dev->dev_stats_ccnt, 0);
> - btrfs_device_data_ordered_init(dev);
> - INIT_RADIX_TREE(&dev->reada_zones, GFP_NOFS & ~__GFP_DIRECT_RECLAIM);
> - INIT_RADIX_TREE(&dev->reada_extents, GFP_NOFS & ~__GFP_DIRECT_RECLAIM);
> - extent_io_tree_init(fs_info, &dev->alloc_state,
> - IO_TREE_DEVICE_ALLOC_STATE, NULL);
> -
> - return dev;
> -}
> -
> static noinline struct btrfs_fs_devices *find_fsid(
> const u8 *fsid, const u8 *metadata_fsid)
> {
> @@ -6856,9 +6818,31 @@ struct btrfs_device *btrfs_alloc_device(struct btrfs_fs_info *fs_info,
> if (WARN_ON(!devid && !fs_info))
> return ERR_PTR(-EINVAL);
>
> - dev = __alloc_device(fs_info);
> - if (IS_ERR(dev))
> - return dev;
> + dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> + if (!dev)
> + return ERR_PTR(-ENOMEM);
> +
> + /*
> + * Preallocate a bio that's always going to be used for flushing device
> + * barriers and matches the device lifespan
> + */
> + dev->flush_bio = bio_kmalloc(GFP_KERNEL, 0);
> + if (!dev->flush_bio) {
> + kfree(dev);
> + return ERR_PTR(-ENOMEM);
> + }
> +
> + INIT_LIST_HEAD(&dev->dev_list);
> + INIT_LIST_HEAD(&dev->dev_alloc_list);
> + INIT_LIST_HEAD(&dev->post_commit_list);
> +
> + atomic_set(&dev->reada_in_flight, 0);
> + atomic_set(&dev->dev_stats_ccnt, 0);
> + btrfs_device_data_ordered_init(dev);
> + INIT_RADIX_TREE(&dev->reada_zones, GFP_NOFS & ~__GFP_DIRECT_RECLAIM);
> + INIT_RADIX_TREE(&dev->reada_extents, GFP_NOFS & ~__GFP_DIRECT_RECLAIM);
> + extent_io_tree_init(fs_info, &dev->alloc_state,
> + IO_TREE_DEVICE_ALLOC_STATE, NULL);
>
> if (devid)
> tmp = *devid;
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 08/10] btrfs: simplify data stripe calculation helpers
2021-07-26 12:15 [PATCH 00/10] Misc small cleanups David Sterba
` (6 preceding siblings ...)
2021-07-26 12:15 ` [PATCH 07/10] btrfs: merge alloc_device helpers David Sterba
@ 2021-07-26 12:15 ` David Sterba
2021-07-26 12:38 ` Qu Wenruo
2021-07-26 12:15 ` [PATCH 09/10] btrfs: constify and cleanup variables comparators David Sterba
2021-07-26 12:15 ` [PATCH 10/10] btrfs: add and use simple page/bio to inode/fs_info helpers David Sterba
9 siblings, 1 reply; 33+ messages in thread
From: David Sterba @ 2021-07-26 12:15 UTC (permalink / raw)
To: linux-btrfs; +Cc: David Sterba
There are two helpers doing the same calculations based on nparity and
ncopies. calc_data_stripes can be simplified into one expression, so far
we don't have profile with both copies and parity, so there's no
effective change. calc_stripe_length should reuse the helper and not
repeat the same calculation.
Signed-off-by: David Sterba <dsterba@suse.com>
---
fs/btrfs/volumes.c | 15 ++-------------
1 file changed, 2 insertions(+), 13 deletions(-)
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index d98e29556d79..78dd013d0ee3 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -3567,10 +3567,7 @@ static u64 calc_data_stripes(u64 type, int num_stripes)
const int ncopies = btrfs_raid_array[index].ncopies;
const int nparity = btrfs_raid_array[index].nparity;
- if (nparity)
- return num_stripes - nparity;
- else
- return num_stripes / ncopies;
+ return (num_stripes - nparity) / ncopies;
}
/* [pstart, pend) */
@@ -6878,15 +6875,7 @@ static void btrfs_report_missing_device(struct btrfs_fs_info *fs_info,
static u64 calc_stripe_length(u64 type, u64 chunk_len, int num_stripes)
{
- int index = btrfs_bg_flags_to_raid_index(type);
- int ncopies = btrfs_raid_array[index].ncopies;
- const int nparity = btrfs_raid_array[index].nparity;
- int data_stripes;
-
- if (nparity)
- data_stripes = num_stripes - nparity;
- else
- data_stripes = num_stripes / ncopies;
+ const int data_stripes = calc_data_stripes(type, num_stripes);
return div_u64(chunk_len, data_stripes);
}
--
2.31.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH 08/10] btrfs: simplify data stripe calculation helpers
2021-07-26 12:15 ` [PATCH 08/10] btrfs: simplify data stripe calculation helpers David Sterba
@ 2021-07-26 12:38 ` Qu Wenruo
2021-07-26 15:06 ` David Sterba
0 siblings, 1 reply; 33+ messages in thread
From: Qu Wenruo @ 2021-07-26 12:38 UTC (permalink / raw)
To: David Sterba, linux-btrfs
On 2021/7/26 下午8:15, David Sterba wrote:
> There are two helpers doing the same calculations based on nparity and
> ncopies. calc_data_stripes can be simplified into one expression, so far
> we don't have profile with both copies and parity, so there's no
> effective change. calc_stripe_length should reuse the helper and not
> repeat the same calculation.
>
> Signed-off-by: David Sterba <dsterba@suse.com>
Reviewed-by: Qu Wenruo <wqu@suse.com>
Just one nitpick inlined below.
> ---
> fs/btrfs/volumes.c | 15 ++-------------
> 1 file changed, 2 insertions(+), 13 deletions(-)
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index d98e29556d79..78dd013d0ee3 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -3567,10 +3567,7 @@ static u64 calc_data_stripes(u64 type, int num_stripes)
> const int ncopies = btrfs_raid_array[index].ncopies;
> const int nparity = btrfs_raid_array[index].nparity;
>
> - if (nparity)
> - return num_stripes - nparity;
> - else
> - return num_stripes / ncopies;
I would prefer an ASSERT() here to be extra sure.
But it's my personal taste (and love for tons of ASSERT()).
Thanks,
Qu
> + return (num_stripes - nparity) / ncopies;
> }
>
> /* [pstart, pend) */
> @@ -6878,15 +6875,7 @@ static void btrfs_report_missing_device(struct btrfs_fs_info *fs_info,
>
> static u64 calc_stripe_length(u64 type, u64 chunk_len, int num_stripes)
> {
> - int index = btrfs_bg_flags_to_raid_index(type);
> - int ncopies = btrfs_raid_array[index].ncopies;
> - const int nparity = btrfs_raid_array[index].nparity;
> - int data_stripes;
> -
> - if (nparity)
> - data_stripes = num_stripes - nparity;
> - else
> - data_stripes = num_stripes / ncopies;
> + const int data_stripes = calc_data_stripes(type, num_stripes);
>
> return div_u64(chunk_len, data_stripes);
> }
>
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH 08/10] btrfs: simplify data stripe calculation helpers
2021-07-26 12:38 ` Qu Wenruo
@ 2021-07-26 15:06 ` David Sterba
2021-07-26 22:23 ` Qu Wenruo
0 siblings, 1 reply; 33+ messages in thread
From: David Sterba @ 2021-07-26 15:06 UTC (permalink / raw)
To: Qu Wenruo; +Cc: David Sterba, linux-btrfs
On Mon, Jul 26, 2021 at 08:38:16PM +0800, Qu Wenruo wrote:
>
>
> On 2021/7/26 下午8:15, David Sterba wrote:
> > There are two helpers doing the same calculations based on nparity and
> > ncopies. calc_data_stripes can be simplified into one expression, so far
> > we don't have profile with both copies and parity, so there's no
> > effective change. calc_stripe_length should reuse the helper and not
> > repeat the same calculation.
> >
> > Signed-off-by: David Sterba <dsterba@suse.com>
>
> Reviewed-by: Qu Wenruo <wqu@suse.com>
>
> Just one nitpick inlined below.
> > ---
> > fs/btrfs/volumes.c | 15 ++-------------
> > 1 file changed, 2 insertions(+), 13 deletions(-)
> >
> > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> > index d98e29556d79..78dd013d0ee3 100644
> > --- a/fs/btrfs/volumes.c
> > +++ b/fs/btrfs/volumes.c
> > @@ -3567,10 +3567,7 @@ static u64 calc_data_stripes(u64 type, int num_stripes)
> > const int ncopies = btrfs_raid_array[index].ncopies;
> > const int nparity = btrfs_raid_array[index].nparity;
> >
> > - if (nparity)
> > - return num_stripes - nparity;
> > - else
> > - return num_stripes / ncopies;
>
> I would prefer an ASSERT() here to be extra sure.
> But it's my personal taste (and love for tons of ASSERT()).
Assert for what exactly? I had a thought about that too but was not sure
what to put there.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 08/10] btrfs: simplify data stripe calculation helpers
2021-07-26 15:06 ` David Sterba
@ 2021-07-26 22:23 ` Qu Wenruo
2021-07-27 8:39 ` David Sterba
0 siblings, 1 reply; 33+ messages in thread
From: Qu Wenruo @ 2021-07-26 22:23 UTC (permalink / raw)
To: dsterba, David Sterba, linux-btrfs
On 2021/7/26 下午11:06, David Sterba wrote:
> On Mon, Jul 26, 2021 at 08:38:16PM +0800, Qu Wenruo wrote:
>>
>>
>> On 2021/7/26 下午8:15, David Sterba wrote:
>>> There are two helpers doing the same calculations based on nparity and
>>> ncopies. calc_data_stripes can be simplified into one expression, so far
>>> we don't have profile with both copies and parity, so there's no
>>> effective change. calc_stripe_length should reuse the helper and not
>>> repeat the same calculation.
>>>
>>> Signed-off-by: David Sterba <dsterba@suse.com>
>>
>> Reviewed-by: Qu Wenruo <wqu@suse.com>
>>
>> Just one nitpick inlined below.
>>> ---
>>> fs/btrfs/volumes.c | 15 ++-------------
>>> 1 file changed, 2 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>>> index d98e29556d79..78dd013d0ee3 100644
>>> --- a/fs/btrfs/volumes.c
>>> +++ b/fs/btrfs/volumes.c
>>> @@ -3567,10 +3567,7 @@ static u64 calc_data_stripes(u64 type, int num_stripes)
>>> const int ncopies = btrfs_raid_array[index].ncopies;
>>> const int nparity = btrfs_raid_array[index].nparity;
>>>
>>> - if (nparity)
>>> - return num_stripes - nparity;
>>> - else
>>> - return num_stripes / ncopies;
>>
>> I would prefer an ASSERT() here to be extra sure.
>> But it's my personal taste (and love for tons of ASSERT()).
>
> Assert for what exactly? I had a thought about that too but was not sure
> what to put there.
>
To ensure we have either non-zero nparity with 1 ncopy or zero nparity
with ncopies > 1.
Thanks,
Qu
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 08/10] btrfs: simplify data stripe calculation helpers
2021-07-26 22:23 ` Qu Wenruo
@ 2021-07-27 8:39 ` David Sterba
2021-07-27 9:32 ` Qu Wenruo
0 siblings, 1 reply; 33+ messages in thread
From: David Sterba @ 2021-07-27 8:39 UTC (permalink / raw)
To: Qu Wenruo; +Cc: dsterba, David Sterba, linux-btrfs
On Tue, Jul 27, 2021 at 06:23:03AM +0800, Qu Wenruo wrote:
> >>> --- a/fs/btrfs/volumes.c
> >>> +++ b/fs/btrfs/volumes.c
> >>> @@ -3567,10 +3567,7 @@ static u64 calc_data_stripes(u64 type, int num_stripes)
> >>> const int ncopies = btrfs_raid_array[index].ncopies;
> >>> const int nparity = btrfs_raid_array[index].nparity;
> >>>
> >>> - if (nparity)
> >>> - return num_stripes - nparity;
> >>> - else
> >>> - return num_stripes / ncopies;
> >>
> >> I would prefer an ASSERT() here to be extra sure.
> >> But it's my personal taste (and love for tons of ASSERT()).
> >
> > Assert for what exactly? I had a thought about that too but was not sure
> > what to put there.
> >
>
> To ensure we have either non-zero nparity with 1 ncopy or zero nparity
> with ncopies > 1.
Yeah but that's statically defined in the raid table, it does not change
so we don't have to verify it on each call. If anything, such
constraints could be verified as _static_assert right after the table
but IMO that's pointless and definitely not a runtime check.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 08/10] btrfs: simplify data stripe calculation helpers
2021-07-27 8:39 ` David Sterba
@ 2021-07-27 9:32 ` Qu Wenruo
0 siblings, 0 replies; 33+ messages in thread
From: Qu Wenruo @ 2021-07-27 9:32 UTC (permalink / raw)
To: dsterba, David Sterba, linux-btrfs
On 2021/7/27 下午4:39, David Sterba wrote:
> On Tue, Jul 27, 2021 at 06:23:03AM +0800, Qu Wenruo wrote:
>>>>> --- a/fs/btrfs/volumes.c
>>>>> +++ b/fs/btrfs/volumes.c
>>>>> @@ -3567,10 +3567,7 @@ static u64 calc_data_stripes(u64 type, int num_stripes)
>>>>> const int ncopies = btrfs_raid_array[index].ncopies;
>>>>> const int nparity = btrfs_raid_array[index].nparity;
>>>>>
>>>>> - if (nparity)
>>>>> - return num_stripes - nparity;
>>>>> - else
>>>>> - return num_stripes / ncopies;
>>>>
>>>> I would prefer an ASSERT() here to be extra sure.
>>>> But it's my personal taste (and love for tons of ASSERT()).
>>>
>>> Assert for what exactly? I had a thought about that too but was not sure
>>> what to put there.
>>>
>>
>> To ensure we have either non-zero nparity with 1 ncopy or zero nparity
>> with ncopies > 1.
>
> Yeah but that's statically defined in the raid table, it does not change
> so we don't have to verify it on each call. If anything, such
> constraints could be verified as _static_assert right after the table
> but IMO that's pointless and definitely not a runtime check.
>
OK, that makes sense.
Reviewed-by: Qu Wenruo <wqu@suse.com>
Thanks,
Qu
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 09/10] btrfs: constify and cleanup variables comparators
2021-07-26 12:15 [PATCH 00/10] Misc small cleanups David Sterba
` (7 preceding siblings ...)
2021-07-26 12:15 ` [PATCH 08/10] btrfs: simplify data stripe calculation helpers David Sterba
@ 2021-07-26 12:15 ` David Sterba
2021-07-26 12:15 ` [PATCH 10/10] btrfs: add and use simple page/bio to inode/fs_info helpers David Sterba
9 siblings, 0 replies; 33+ messages in thread
From: David Sterba @ 2021-07-26 12:15 UTC (permalink / raw)
To: linux-btrfs; +Cc: David Sterba
Comparators just read the data and thus get const parameters. This
should be also preserved by the local variables, update all comparators
passed to sort or bsearch.
Cleanups:
- unnecessary casts are dropped
- btrfs_cmp_device_free_bytes is cleaned up to follow the common pattern
and 'inline' is dropped as the function address is taken
Signed-off-by: David Sterba <dsterba@suse.com>
---
fs/btrfs/raid56.c | 8 ++++----
fs/btrfs/send.c | 6 +++---
fs/btrfs/super.c | 13 ++++++-------
fs/btrfs/tree-log.c | 2 +-
fs/btrfs/volumes.c | 2 +-
5 files changed, 15 insertions(+), 16 deletions(-)
diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
index a40a45a007d4..d8d268ca8aa7 100644
--- a/fs/btrfs/raid56.c
+++ b/fs/btrfs/raid56.c
@@ -1636,10 +1636,10 @@ struct btrfs_plug_cb {
static int plug_cmp(void *priv, const struct list_head *a,
const struct list_head *b)
{
- struct btrfs_raid_bio *ra = container_of(a, struct btrfs_raid_bio,
- plug_list);
- struct btrfs_raid_bio *rb = container_of(b, struct btrfs_raid_bio,
- plug_list);
+ const struct btrfs_raid_bio *ra = container_of(a, struct btrfs_raid_bio,
+ plug_list);
+ const struct btrfs_raid_bio *rb = container_of(b, struct btrfs_raid_bio,
+ plug_list);
u64 a_sector = ra->bio_list.head->bi_iter.bi_sector;
u64 b_sector = rb->bio_list.head->bi_iter.bi_sector;
diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index 6ac37ae6c811..75cff564dedf 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -1198,7 +1198,7 @@ struct backref_ctx {
static int __clone_root_cmp_bsearch(const void *key, const void *elt)
{
u64 root = (u64)(uintptr_t)key;
- struct clone_root *cr = (struct clone_root *)elt;
+ const struct clone_root *cr = elt;
if (root < cr->root->root_key.objectid)
return -1;
@@ -1209,8 +1209,8 @@ static int __clone_root_cmp_bsearch(const void *key, const void *elt)
static int __clone_root_cmp_sort(const void *e1, const void *e2)
{
- struct clone_root *cr1 = (struct clone_root *)e1;
- struct clone_root *cr2 = (struct clone_root *)e2;
+ const struct clone_root *cr1 = e1;
+ const struct clone_root *cr2 = e2;
if (cr1->root->root_key.objectid < cr2->root->root_key.objectid)
return -1;
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index d07b18b2b250..35ff142ad242 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -2096,16 +2096,15 @@ static int btrfs_remount(struct super_block *sb, int *flags, char *data)
}
/* Used to sort the devices by max_avail(descending sort) */
-static inline int btrfs_cmp_device_free_bytes(const void *dev_info1,
- const void *dev_info2)
+static int btrfs_cmp_device_free_bytes(const void *a, const void *b)
{
- if (((struct btrfs_device_info *)dev_info1)->max_avail >
- ((struct btrfs_device_info *)dev_info2)->max_avail)
+ const struct btrfs_device_info *dev_info1 = a;
+ const struct btrfs_device_info *dev_info2 = b;
+
+ if (dev_info1->max_avail > dev_info2->max_avail)
return -1;
- else if (((struct btrfs_device_info *)dev_info1)->max_avail <
- ((struct btrfs_device_info *)dev_info2)->max_avail)
+ else if (dev_info1->max_avail < dev_info2->max_avail)
return 1;
- else
return 0;
}
diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index c2ebe700d6e2..d3630f404544 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -4191,7 +4191,7 @@ static noinline int copy_items(struct btrfs_trans_handle *trans,
static int extent_cmp(void *priv, const struct list_head *a,
const struct list_head *b)
{
- struct extent_map *em1, *em2;
+ const struct extent_map *em1, *em2;
em1 = list_entry(a, struct extent_map, list);
em2 = list_entry(b, struct extent_map, list);
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 78dd013d0ee3..e60e084763b0 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1215,7 +1215,7 @@ static int open_fs_devices(struct btrfs_fs_devices *fs_devices,
static int devid_cmp(void *priv, const struct list_head *a,
const struct list_head *b)
{
- struct btrfs_device *dev1, *dev2;
+ const struct btrfs_device *dev1, *dev2;
dev1 = list_entry(a, struct btrfs_device, dev_list);
dev2 = list_entry(b, struct btrfs_device, dev_list);
--
2.31.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH 10/10] btrfs: add and use simple page/bio to inode/fs_info helpers
2021-07-26 12:15 [PATCH 00/10] Misc small cleanups David Sterba
` (8 preceding siblings ...)
2021-07-26 12:15 ` [PATCH 09/10] btrfs: constify and cleanup variables comparators David Sterba
@ 2021-07-26 12:15 ` David Sterba
2021-07-26 12:41 ` Qu Wenruo
2021-07-27 15:03 ` [PATCH v2 " David Sterba
9 siblings, 2 replies; 33+ messages in thread
From: David Sterba @ 2021-07-26 12:15 UTC (permalink / raw)
To: linux-btrfs; +Cc: David Sterba
We have lots of places where we want to obtain inode from page, fs_info
from page and open code the pointer chains.
Add helpers for the most common actions where the types match. There are
still unconverted cases that don't have btrfs_inode and need this
conversion first.
Signed-off-by: David Sterba <dsterba@suse.com>
---
fs/btrfs/disk-io.c | 24 +++++++++++++-----------
fs/btrfs/extent_io.c | 33 ++++++++++++++++-----------------
fs/btrfs/inode.c | 4 ++--
fs/btrfs/misc.h | 4 ++++
4 files changed, 35 insertions(+), 30 deletions(-)
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index b117dd3b8172..cdb7791b00d7 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -19,6 +19,7 @@
#include <linux/sched/mm.h>
#include <asm/unaligned.h>
#include <crypto/hash.h>
+#include "misc.h"
#include "ctree.h"
#include "disk-io.h"
#include "transaction.h"
@@ -633,7 +634,7 @@ static int validate_extent_buffer(struct extent_buffer *eb)
static int validate_subpage_buffer(struct page *page, u64 start, u64 end,
int mirror)
{
- struct btrfs_fs_info *fs_info = btrfs_sb(page->mapping->host->i_sb);
+ struct btrfs_fs_info *fs_info = page_to_fs_info(page);
struct extent_buffer *eb;
bool reads_done;
int ret = 0;
@@ -693,7 +694,7 @@ int btrfs_validate_metadata_buffer(struct btrfs_io_bio *io_bio,
ASSERT(page->private);
- if (btrfs_sb(page->mapping->host->i_sb)->sectorsize < PAGE_SIZE)
+ if (page_to_fs_info(page)->sectorsize < PAGE_SIZE)
return validate_subpage_buffer(page, start, end, mirror);
eb = (struct extent_buffer *)page->private;
@@ -876,14 +877,14 @@ blk_status_t btrfs_wq_submit_bio(struct inode *inode, struct bio *bio,
static blk_status_t btree_csum_one_bio(struct bio *bio)
{
struct bio_vec *bvec;
- struct btrfs_root *root;
int ret = 0;
struct bvec_iter_all iter_all;
ASSERT(!bio_flagged(bio, BIO_CLONED));
bio_for_each_segment_all(bvec, bio, iter_all) {
- root = BTRFS_I(bvec->bv_page->mapping->host)->root;
- ret = csum_dirty_buffer(root->fs_info, bvec);
+ struct btrfs_fs_info *fs_info = page_to_fs_info(bvec->bv_page);
+
+ ret = csum_dirty_buffer(fs_info, bvec);
if (ret)
break;
}
@@ -1010,11 +1011,13 @@ static void btree_invalidatepage(struct page *page, unsigned int offset,
unsigned int length)
{
struct extent_io_tree *tree;
- tree = &BTRFS_I(page->mapping->host)->io_tree;
+ struct btrfs_inode *inode = page_to_inode(page);
+
+ tree = &inode->io_tree;
extent_invalidatepage(tree, page, offset);
btree_releasepage(page, GFP_NOFS);
if (PagePrivate(page)) {
- btrfs_warn(BTRFS_I(page->mapping->host)->root->fs_info,
+ btrfs_warn(inode->root->fs_info,
"page private not zero on page %llu",
(unsigned long long)page_offset(page));
detach_page_private(page);
@@ -1024,7 +1027,7 @@ static void btree_invalidatepage(struct page *page, unsigned int offset,
static int btree_set_page_dirty(struct page *page)
{
#ifdef DEBUG
- struct btrfs_fs_info *fs_info = btrfs_sb(page->mapping->host->i_sb);
+ struct btrfs_fs_info *fs_info = page_to_fs_info(page);
struct btrfs_subpage *subpage;
struct extent_buffer *eb;
int cur_bit = 0;
@@ -4437,14 +4440,13 @@ int btrfs_buffer_uptodate(struct extent_buffer *buf, u64 parent_transid,
int atomic)
{
int ret;
- struct inode *btree_inode = buf->pages[0]->mapping->host;
+ struct btrfs_inode *inode = page_to_inode(buf->pages[0]);
ret = extent_buffer_uptodate(buf);
if (!ret)
return ret;
- ret = verify_parent_transid(&BTRFS_I(btree_inode)->io_tree, buf,
- parent_transid, atomic);
+ ret = verify_parent_transid(&inode->io_tree, buf, parent_transid, atomic);
if (ret == -EAGAIN)
return ret;
return !ret;
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index f7e58c304fc9..d8ce7588de77 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -2682,7 +2682,7 @@ int btrfs_repair_one_sector(struct inode *inode,
static void end_page_read(struct page *page, bool uptodate, u64 start, u32 len)
{
- struct btrfs_fs_info *fs_info = btrfs_sb(page->mapping->host->i_sb);
+ struct btrfs_fs_info *fs_info = page_to_fs_info(page);
ASSERT(page_offset(page) <= start &&
start + len <= page_offset(page) + PAGE_SIZE);
@@ -2783,7 +2783,7 @@ void end_extent_writepage(struct page *page, int err, u64 start, u64 end)
int ret = 0;
ASSERT(page && page->mapping);
- inode = BTRFS_I(page->mapping->host);
+ inode = page_to_inode(page);
btrfs_writepage_endio_finish_ordered(inode, page, start, end, uptodate);
if (!uptodate) {
@@ -2815,8 +2815,8 @@ static void end_bio_extent_writepage(struct bio *bio)
ASSERT(!bio_flagged(bio, BIO_CLONED));
bio_for_each_segment_all(bvec, bio, iter_all) {
struct page *page = bvec->bv_page;
- struct inode *inode = page->mapping->host;
- struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
+ struct btrfs_inode *inode = page_to_inode(page);
+ struct btrfs_fs_info *fs_info = inode->root->fs_info;
const u32 sectorsize = fs_info->sectorsize;
/* Our read/write should always be sector aligned. */
@@ -2833,7 +2833,7 @@ static void end_bio_extent_writepage(struct bio *bio)
end = start + bvec->bv_len - 1;
if (first_bvec) {
- btrfs_record_physical_zoned(inode, start, bio);
+ btrfs_record_physical_zoned(&inode->vfs_inode, start, bio);
first_bvec = false;
}
@@ -3306,7 +3306,7 @@ static int submit_extent_page(unsigned int opf,
int ret = 0;
struct bio *bio;
size_t io_size = min_t(size_t, size, PAGE_SIZE);
- struct btrfs_inode *inode = BTRFS_I(page->mapping->host);
+ struct btrfs_inode *inode = page_to_inode(page);
struct extent_io_tree *tree = &inode->io_tree;
struct btrfs_fs_info *fs_info = inode->root->fs_info;
@@ -3334,7 +3334,7 @@ static int submit_extent_page(unsigned int opf,
bio_add_page(bio, page, io_size, pg_offset);
bio->bi_end_io = end_io_func;
bio->bi_private = tree;
- bio->bi_write_hint = page->mapping->host->i_write_hint;
+ bio->bi_write_hint = inode->vfs_inode.i_write_hint;
bio->bi_opf = opf;
if (wbc) {
struct block_device *bdev;
@@ -3410,8 +3410,7 @@ int set_page_extent_mapped(struct page *page)
if (PagePrivate(page))
return 0;
- fs_info = btrfs_sb(page->mapping->host->i_sb);
-
+ fs_info = page_to_fs_info(page);
if (fs_info->sectorsize < PAGE_SIZE)
return btrfs_attach_subpage(fs_info, page, BTRFS_SUBPAGE_DATA);
@@ -3428,7 +3427,7 @@ void clear_page_extent_mapped(struct page *page)
if (!PagePrivate(page))
return;
- fs_info = btrfs_sb(page->mapping->host->i_sb);
+ fs_info = page_to_fs_info(page);
if (fs_info->sectorsize < PAGE_SIZE)
return btrfs_detach_subpage(fs_info, page);
@@ -3670,7 +3669,7 @@ static inline void contiguous_readpages(struct page *pages[], int nr_pages,
struct btrfs_bio_ctrl *bio_ctrl,
u64 *prev_em_start)
{
- struct btrfs_inode *inode = BTRFS_I(pages[0]->mapping->host);
+ struct btrfs_inode *inode = page_to_inode(pages[0]);
int index;
btrfs_lock_and_flush_ordered_range(inode, start, end, NULL);
@@ -4259,7 +4258,7 @@ static void end_bio_subpage_eb_writepage(struct bio *bio)
struct bio_vec *bvec;
struct bvec_iter_all iter_all;
- fs_info = btrfs_sb(bio_first_page_all(bio)->mapping->host->i_sb);
+ fs_info = bio_to_fs_info(bio);
ASSERT(fs_info->sectorsize < PAGE_SIZE);
ASSERT(!bio_flagged(bio, BIO_CLONED));
@@ -4481,7 +4480,7 @@ static int submit_eb_subpage(struct page *page,
struct writeback_control *wbc,
struct extent_page_data *epd)
{
- struct btrfs_fs_info *fs_info = btrfs_sb(page->mapping->host->i_sb);
+ struct btrfs_fs_info *fs_info = page_to_fs_info(page);
int submitted = 0;
u64 page_start = page_offset(page);
int bit_start = 0;
@@ -4587,7 +4586,7 @@ static int submit_eb_page(struct page *page, struct writeback_control *wbc,
if (!PagePrivate(page))
return 0;
- if (btrfs_sb(page->mapping->host->i_sb)->sectorsize < PAGE_SIZE)
+ if (page_to_fs_info(page)->sectorsize < PAGE_SIZE)
return submit_eb_subpage(page, wbc, epd);
spin_lock(&mapping->private_lock);
@@ -5123,7 +5122,7 @@ int try_release_extent_mapping(struct page *page, gfp_t mask)
struct extent_map *em;
u64 start = page_offset(page);
u64 end = start + PAGE_SIZE - 1;
- struct btrfs_inode *btrfs_inode = BTRFS_I(page->mapping->host);
+ struct btrfs_inode *btrfs_inode = page_to_inode(page);
struct extent_io_tree *tree = &btrfs_inode->io_tree;
struct extent_map_tree *map = &btrfs_inode->extent_tree;
@@ -7081,7 +7080,7 @@ static struct extent_buffer *get_next_extent_buffer(
static int try_release_subpage_extent_buffer(struct page *page)
{
- struct btrfs_fs_info *fs_info = btrfs_sb(page->mapping->host->i_sb);
+ struct btrfs_fs_info *fs_info = page_to_fs_info(page);
u64 cur = page_offset(page);
const u64 end = page_offset(page) + PAGE_SIZE;
int ret;
@@ -7153,7 +7152,7 @@ int try_release_extent_buffer(struct page *page)
{
struct extent_buffer *eb;
- if (btrfs_sb(page->mapping->host->i_sb)->sectorsize < PAGE_SIZE)
+ if (page_to_fs_info(page)->sectorsize < PAGE_SIZE)
return try_release_subpage_extent_buffer(page);
/*
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index ba0bba9f5505..19db7f18da42 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -8348,7 +8348,7 @@ static int btrfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
int btrfs_readpage(struct file *file, struct page *page)
{
- struct btrfs_inode *inode = BTRFS_I(page->mapping->host);
+ struct btrfs_inode *inode = page_to_inode(page);
u64 start = page_offset(page);
u64 end = start + PAGE_SIZE - 1;
struct btrfs_bio_ctrl bio_ctrl = { 0 };
@@ -8443,7 +8443,7 @@ static int btrfs_migratepage(struct address_space *mapping,
static void btrfs_invalidatepage(struct page *page, unsigned int offset,
unsigned int length)
{
- struct btrfs_inode *inode = BTRFS_I(page->mapping->host);
+ struct btrfs_inode *inode = page_to_inode(page);
struct btrfs_fs_info *fs_info = inode->root->fs_info;
struct extent_io_tree *tree = &inode->io_tree;
struct extent_state *cached_state = NULL;
diff --git a/fs/btrfs/misc.h b/fs/btrfs/misc.h
index 6461ebc3a1c1..cb6dc975a159 100644
--- a/fs/btrfs/misc.h
+++ b/fs/btrfs/misc.h
@@ -10,6 +10,10 @@
#define in_range(b, first, len) ((b) >= (first) && (b) < (first) + (len))
+#define page_to_fs_info(page) (BTRFS_I((page)->mapping->host)->root->fs_info)
+#define page_to_inode(page) (BTRFS_I((page)->mapping->host))
+#define bio_to_fs_info(bio) (page_to_fs_info(bio_first_page_all(bio)))
+
static inline void cond_wake_up(struct wait_queue_head *wq)
{
/*
--
2.31.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH 10/10] btrfs: add and use simple page/bio to inode/fs_info helpers
2021-07-26 12:15 ` [PATCH 10/10] btrfs: add and use simple page/bio to inode/fs_info helpers David Sterba
@ 2021-07-26 12:41 ` Qu Wenruo
2021-07-26 15:09 ` David Sterba
2021-07-27 15:03 ` [PATCH v2 " David Sterba
1 sibling, 1 reply; 33+ messages in thread
From: Qu Wenruo @ 2021-07-26 12:41 UTC (permalink / raw)
To: David Sterba, linux-btrfs
On 2021/7/26 下午8:15, David Sterba wrote:
> We have lots of places where we want to obtain inode from page, fs_info
> from page and open code the pointer chains.
All those inode/fs_info grabbing from just a page is dangerous.
If an anonymous page is passed in unintentionally, it can easily crash
the system.
Thus at least some ASSERT() here is a must to me.
Thanks,
Qu
>
> Add helpers for the most common actions where the types match. There are
> still unconverted cases that don't have btrfs_inode and need this
> conversion first.
>
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
> fs/btrfs/disk-io.c | 24 +++++++++++++-----------
> fs/btrfs/extent_io.c | 33 ++++++++++++++++-----------------
> fs/btrfs/inode.c | 4 ++--
> fs/btrfs/misc.h | 4 ++++
> 4 files changed, 35 insertions(+), 30 deletions(-)
>
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index b117dd3b8172..cdb7791b00d7 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -19,6 +19,7 @@
> #include <linux/sched/mm.h>
> #include <asm/unaligned.h>
> #include <crypto/hash.h>
> +#include "misc.h"
> #include "ctree.h"
> #include "disk-io.h"
> #include "transaction.h"
> @@ -633,7 +634,7 @@ static int validate_extent_buffer(struct extent_buffer *eb)
> static int validate_subpage_buffer(struct page *page, u64 start, u64 end,
> int mirror)
> {
> - struct btrfs_fs_info *fs_info = btrfs_sb(page->mapping->host->i_sb);
> + struct btrfs_fs_info *fs_info = page_to_fs_info(page);
> struct extent_buffer *eb;
> bool reads_done;
> int ret = 0;
> @@ -693,7 +694,7 @@ int btrfs_validate_metadata_buffer(struct btrfs_io_bio *io_bio,
>
> ASSERT(page->private);
>
> - if (btrfs_sb(page->mapping->host->i_sb)->sectorsize < PAGE_SIZE)
> + if (page_to_fs_info(page)->sectorsize < PAGE_SIZE)
> return validate_subpage_buffer(page, start, end, mirror);
>
> eb = (struct extent_buffer *)page->private;
> @@ -876,14 +877,14 @@ blk_status_t btrfs_wq_submit_bio(struct inode *inode, struct bio *bio,
> static blk_status_t btree_csum_one_bio(struct bio *bio)
> {
> struct bio_vec *bvec;
> - struct btrfs_root *root;
> int ret = 0;
> struct bvec_iter_all iter_all;
>
> ASSERT(!bio_flagged(bio, BIO_CLONED));
> bio_for_each_segment_all(bvec, bio, iter_all) {
> - root = BTRFS_I(bvec->bv_page->mapping->host)->root;
> - ret = csum_dirty_buffer(root->fs_info, bvec);
> + struct btrfs_fs_info *fs_info = page_to_fs_info(bvec->bv_page);
> +
> + ret = csum_dirty_buffer(fs_info, bvec);
> if (ret)
> break;
> }
> @@ -1010,11 +1011,13 @@ static void btree_invalidatepage(struct page *page, unsigned int offset,
> unsigned int length)
> {
> struct extent_io_tree *tree;
> - tree = &BTRFS_I(page->mapping->host)->io_tree;
> + struct btrfs_inode *inode = page_to_inode(page);
> +
> + tree = &inode->io_tree;
> extent_invalidatepage(tree, page, offset);
> btree_releasepage(page, GFP_NOFS);
> if (PagePrivate(page)) {
> - btrfs_warn(BTRFS_I(page->mapping->host)->root->fs_info,
> + btrfs_warn(inode->root->fs_info,
> "page private not zero on page %llu",
> (unsigned long long)page_offset(page));
> detach_page_private(page);
> @@ -1024,7 +1027,7 @@ static void btree_invalidatepage(struct page *page, unsigned int offset,
> static int btree_set_page_dirty(struct page *page)
> {
> #ifdef DEBUG
> - struct btrfs_fs_info *fs_info = btrfs_sb(page->mapping->host->i_sb);
> + struct btrfs_fs_info *fs_info = page_to_fs_info(page);
> struct btrfs_subpage *subpage;
> struct extent_buffer *eb;
> int cur_bit = 0;
> @@ -4437,14 +4440,13 @@ int btrfs_buffer_uptodate(struct extent_buffer *buf, u64 parent_transid,
> int atomic)
> {
> int ret;
> - struct inode *btree_inode = buf->pages[0]->mapping->host;
> + struct btrfs_inode *inode = page_to_inode(buf->pages[0]);
>
> ret = extent_buffer_uptodate(buf);
> if (!ret)
> return ret;
>
> - ret = verify_parent_transid(&BTRFS_I(btree_inode)->io_tree, buf,
> - parent_transid, atomic);
> + ret = verify_parent_transid(&inode->io_tree, buf, parent_transid, atomic);
> if (ret == -EAGAIN)
> return ret;
> return !ret;
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index f7e58c304fc9..d8ce7588de77 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -2682,7 +2682,7 @@ int btrfs_repair_one_sector(struct inode *inode,
>
> static void end_page_read(struct page *page, bool uptodate, u64 start, u32 len)
> {
> - struct btrfs_fs_info *fs_info = btrfs_sb(page->mapping->host->i_sb);
> + struct btrfs_fs_info *fs_info = page_to_fs_info(page);
>
> ASSERT(page_offset(page) <= start &&
> start + len <= page_offset(page) + PAGE_SIZE);
> @@ -2783,7 +2783,7 @@ void end_extent_writepage(struct page *page, int err, u64 start, u64 end)
> int ret = 0;
>
> ASSERT(page && page->mapping);
> - inode = BTRFS_I(page->mapping->host);
> + inode = page_to_inode(page);
> btrfs_writepage_endio_finish_ordered(inode, page, start, end, uptodate);
>
> if (!uptodate) {
> @@ -2815,8 +2815,8 @@ static void end_bio_extent_writepage(struct bio *bio)
> ASSERT(!bio_flagged(bio, BIO_CLONED));
> bio_for_each_segment_all(bvec, bio, iter_all) {
> struct page *page = bvec->bv_page;
> - struct inode *inode = page->mapping->host;
> - struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
> + struct btrfs_inode *inode = page_to_inode(page);
> + struct btrfs_fs_info *fs_info = inode->root->fs_info;
> const u32 sectorsize = fs_info->sectorsize;
>
> /* Our read/write should always be sector aligned. */
> @@ -2833,7 +2833,7 @@ static void end_bio_extent_writepage(struct bio *bio)
> end = start + bvec->bv_len - 1;
>
> if (first_bvec) {
> - btrfs_record_physical_zoned(inode, start, bio);
> + btrfs_record_physical_zoned(&inode->vfs_inode, start, bio);
> first_bvec = false;
> }
>
> @@ -3306,7 +3306,7 @@ static int submit_extent_page(unsigned int opf,
> int ret = 0;
> struct bio *bio;
> size_t io_size = min_t(size_t, size, PAGE_SIZE);
> - struct btrfs_inode *inode = BTRFS_I(page->mapping->host);
> + struct btrfs_inode *inode = page_to_inode(page);
> struct extent_io_tree *tree = &inode->io_tree;
> struct btrfs_fs_info *fs_info = inode->root->fs_info;
>
> @@ -3334,7 +3334,7 @@ static int submit_extent_page(unsigned int opf,
> bio_add_page(bio, page, io_size, pg_offset);
> bio->bi_end_io = end_io_func;
> bio->bi_private = tree;
> - bio->bi_write_hint = page->mapping->host->i_write_hint;
> + bio->bi_write_hint = inode->vfs_inode.i_write_hint;
> bio->bi_opf = opf;
> if (wbc) {
> struct block_device *bdev;
> @@ -3410,8 +3410,7 @@ int set_page_extent_mapped(struct page *page)
> if (PagePrivate(page))
> return 0;
>
> - fs_info = btrfs_sb(page->mapping->host->i_sb);
> -
> + fs_info = page_to_fs_info(page);
> if (fs_info->sectorsize < PAGE_SIZE)
> return btrfs_attach_subpage(fs_info, page, BTRFS_SUBPAGE_DATA);
>
> @@ -3428,7 +3427,7 @@ void clear_page_extent_mapped(struct page *page)
> if (!PagePrivate(page))
> return;
>
> - fs_info = btrfs_sb(page->mapping->host->i_sb);
> + fs_info = page_to_fs_info(page);
> if (fs_info->sectorsize < PAGE_SIZE)
> return btrfs_detach_subpage(fs_info, page);
>
> @@ -3670,7 +3669,7 @@ static inline void contiguous_readpages(struct page *pages[], int nr_pages,
> struct btrfs_bio_ctrl *bio_ctrl,
> u64 *prev_em_start)
> {
> - struct btrfs_inode *inode = BTRFS_I(pages[0]->mapping->host);
> + struct btrfs_inode *inode = page_to_inode(pages[0]);
> int index;
>
> btrfs_lock_and_flush_ordered_range(inode, start, end, NULL);
> @@ -4259,7 +4258,7 @@ static void end_bio_subpage_eb_writepage(struct bio *bio)
> struct bio_vec *bvec;
> struct bvec_iter_all iter_all;
>
> - fs_info = btrfs_sb(bio_first_page_all(bio)->mapping->host->i_sb);
> + fs_info = bio_to_fs_info(bio);
> ASSERT(fs_info->sectorsize < PAGE_SIZE);
>
> ASSERT(!bio_flagged(bio, BIO_CLONED));
> @@ -4481,7 +4480,7 @@ static int submit_eb_subpage(struct page *page,
> struct writeback_control *wbc,
> struct extent_page_data *epd)
> {
> - struct btrfs_fs_info *fs_info = btrfs_sb(page->mapping->host->i_sb);
> + struct btrfs_fs_info *fs_info = page_to_fs_info(page);
> int submitted = 0;
> u64 page_start = page_offset(page);
> int bit_start = 0;
> @@ -4587,7 +4586,7 @@ static int submit_eb_page(struct page *page, struct writeback_control *wbc,
> if (!PagePrivate(page))
> return 0;
>
> - if (btrfs_sb(page->mapping->host->i_sb)->sectorsize < PAGE_SIZE)
> + if (page_to_fs_info(page)->sectorsize < PAGE_SIZE)
> return submit_eb_subpage(page, wbc, epd);
>
> spin_lock(&mapping->private_lock);
> @@ -5123,7 +5122,7 @@ int try_release_extent_mapping(struct page *page, gfp_t mask)
> struct extent_map *em;
> u64 start = page_offset(page);
> u64 end = start + PAGE_SIZE - 1;
> - struct btrfs_inode *btrfs_inode = BTRFS_I(page->mapping->host);
> + struct btrfs_inode *btrfs_inode = page_to_inode(page);
> struct extent_io_tree *tree = &btrfs_inode->io_tree;
> struct extent_map_tree *map = &btrfs_inode->extent_tree;
>
> @@ -7081,7 +7080,7 @@ static struct extent_buffer *get_next_extent_buffer(
>
> static int try_release_subpage_extent_buffer(struct page *page)
> {
> - struct btrfs_fs_info *fs_info = btrfs_sb(page->mapping->host->i_sb);
> + struct btrfs_fs_info *fs_info = page_to_fs_info(page);
> u64 cur = page_offset(page);
> const u64 end = page_offset(page) + PAGE_SIZE;
> int ret;
> @@ -7153,7 +7152,7 @@ int try_release_extent_buffer(struct page *page)
> {
> struct extent_buffer *eb;
>
> - if (btrfs_sb(page->mapping->host->i_sb)->sectorsize < PAGE_SIZE)
> + if (page_to_fs_info(page)->sectorsize < PAGE_SIZE)
> return try_release_subpage_extent_buffer(page);
>
> /*
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index ba0bba9f5505..19db7f18da42 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -8348,7 +8348,7 @@ static int btrfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
>
> int btrfs_readpage(struct file *file, struct page *page)
> {
> - struct btrfs_inode *inode = BTRFS_I(page->mapping->host);
> + struct btrfs_inode *inode = page_to_inode(page);
> u64 start = page_offset(page);
> u64 end = start + PAGE_SIZE - 1;
> struct btrfs_bio_ctrl bio_ctrl = { 0 };
> @@ -8443,7 +8443,7 @@ static int btrfs_migratepage(struct address_space *mapping,
> static void btrfs_invalidatepage(struct page *page, unsigned int offset,
> unsigned int length)
> {
> - struct btrfs_inode *inode = BTRFS_I(page->mapping->host);
> + struct btrfs_inode *inode = page_to_inode(page);
> struct btrfs_fs_info *fs_info = inode->root->fs_info;
> struct extent_io_tree *tree = &inode->io_tree;
> struct extent_state *cached_state = NULL;
> diff --git a/fs/btrfs/misc.h b/fs/btrfs/misc.h
> index 6461ebc3a1c1..cb6dc975a159 100644
> --- a/fs/btrfs/misc.h
> +++ b/fs/btrfs/misc.h
> @@ -10,6 +10,10 @@
>
> #define in_range(b, first, len) ((b) >= (first) && (b) < (first) + (len))
>
> +#define page_to_fs_info(page) (BTRFS_I((page)->mapping->host)->root->fs_info)
> +#define page_to_inode(page) (BTRFS_I((page)->mapping->host))
> +#define bio_to_fs_info(bio) (page_to_fs_info(bio_first_page_all(bio)))
> +
> static inline void cond_wake_up(struct wait_queue_head *wq)
> {
> /*
>
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH 10/10] btrfs: add and use simple page/bio to inode/fs_info helpers
2021-07-26 12:41 ` Qu Wenruo
@ 2021-07-26 15:09 ` David Sterba
2021-07-26 22:26 ` Qu Wenruo
0 siblings, 1 reply; 33+ messages in thread
From: David Sterba @ 2021-07-26 15:09 UTC (permalink / raw)
To: Qu Wenruo; +Cc: David Sterba, linux-btrfs
On Mon, Jul 26, 2021 at 08:41:57PM +0800, Qu Wenruo wrote:
>
>
> On 2021/7/26 下午8:15, David Sterba wrote:
> > We have lots of places where we want to obtain inode from page, fs_info
> > from page and open code the pointer chains.
>
> All those inode/fs_info grabbing from just a page is dangerous.
>
> If an anonymous page is passed in unintentionally, it can easily crash
> the system.
>
> Thus at least some ASSERT() here is a must to me.
But we can only check if the pointer is valid, any page can have a valid
pointer but not our fs_info. If it crashes on an unexpected page than
what can we do in the code anyway?
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 10/10] btrfs: add and use simple page/bio to inode/fs_info helpers
2021-07-26 15:09 ` David Sterba
@ 2021-07-26 22:26 ` Qu Wenruo
2021-07-27 8:45 ` David Sterba
0 siblings, 1 reply; 33+ messages in thread
From: Qu Wenruo @ 2021-07-26 22:26 UTC (permalink / raw)
To: dsterba, David Sterba, linux-btrfs
On 2021/7/26 下午11:09, David Sterba wrote:
> On Mon, Jul 26, 2021 at 08:41:57PM +0800, Qu Wenruo wrote:
>>
>>
>> On 2021/7/26 下午8:15, David Sterba wrote:
>>> We have lots of places where we want to obtain inode from page, fs_info
>>> from page and open code the pointer chains.
>>
>> All those inode/fs_info grabbing from just a page is dangerous.
>>
>> If an anonymous page is passed in unintentionally, it can easily crash
>> the system.
>>
>> Thus at least some ASSERT() here is a must to me.
>
> But we can only check if the pointer is valid, any page can have a valid
> pointer but not our fs_info. If it crashes on an unexpected page than
> what can we do in the code anyway?
>
What I mean is to check page->mapping for the page passed in.
Indeed we can't do anything when we hit a page with NULL mapping
pointer, but that's a code bug.
An ASSERT() would make us developer aware what's going wrong and to fix
the bug.
Thanks,
Qu
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 10/10] btrfs: add and use simple page/bio to inode/fs_info helpers
2021-07-26 22:26 ` Qu Wenruo
@ 2021-07-27 8:45 ` David Sterba
2021-07-27 9:42 ` Qu Wenruo
0 siblings, 1 reply; 33+ messages in thread
From: David Sterba @ 2021-07-27 8:45 UTC (permalink / raw)
To: Qu Wenruo; +Cc: dsterba, David Sterba, linux-btrfs
On Tue, Jul 27, 2021 at 06:26:47AM +0800, Qu Wenruo wrote:
> On 2021/7/26 下午11:09, David Sterba wrote:
> > On Mon, Jul 26, 2021 at 08:41:57PM +0800, Qu Wenruo wrote:
> >>
> >>
> >> On 2021/7/26 下午8:15, David Sterba wrote:
> >>> We have lots of places where we want to obtain inode from page, fs_info
> >>> from page and open code the pointer chains.
> >>
> >> All those inode/fs_info grabbing from just a page is dangerous.
> >>
> >> If an anonymous page is passed in unintentionally, it can easily crash
> >> the system.
> >>
> >> Thus at least some ASSERT() here is a must to me.
> >
> > But we can only check if the pointer is valid, any page can have a valid
> > pointer but not our fs_info. If it crashes on an unexpected page than
> > what can we do in the code anyway?
>
> What I mean is to check page->mapping for the page passed in.
>
> Indeed we can't do anything when we hit a page with NULL mapping
> pointer, but that's a code bug.
> An ASSERT() would make us developer aware what's going wrong and to fix
> the bug.
The assert is a more verbose crash, so that's slightly more developer
friendly but I'm still not convinced it's worth the assert. Right now
the macros are not static inlines so they don't need full definitions of
page and mapping and the other types. Embedding the asserts into macros
would look like
({ ASSERT(page); ASSERT(page->mapping); page->mapping->host; })
Or perhaps also page with a temporary variable to avoid multiple
evaluations.
The helpers are used in a handful of places, if we really care about
consistency of the assertions, something like assert_page_ok(page) would
have to be in each function that gets the page from other subsystems.
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH 10/10] btrfs: add and use simple page/bio to inode/fs_info helpers
2021-07-27 8:45 ` David Sterba
@ 2021-07-27 9:42 ` Qu Wenruo
2021-07-27 14:44 ` David Sterba
0 siblings, 1 reply; 33+ messages in thread
From: Qu Wenruo @ 2021-07-27 9:42 UTC (permalink / raw)
To: dsterba, Qu Wenruo, David Sterba, linux-btrfs
On 2021/7/27 下午4:45, David Sterba wrote:
> On Tue, Jul 27, 2021 at 06:26:47AM +0800, Qu Wenruo wrote:
>> On 2021/7/26 下午11:09, David Sterba wrote:
>>> On Mon, Jul 26, 2021 at 08:41:57PM +0800, Qu Wenruo wrote:
>>>>
>>>>
>>>> On 2021/7/26 下午8:15, David Sterba wrote:
>>>>> We have lots of places where we want to obtain inode from page, fs_info
>>>>> from page and open code the pointer chains.
>>>>
>>>> All those inode/fs_info grabbing from just a page is dangerous.
>>>>
>>>> If an anonymous page is passed in unintentionally, it can easily crash
>>>> the system.
>>>>
>>>> Thus at least some ASSERT() here is a must to me.
>>>
>>> But we can only check if the pointer is valid, any page can have a valid
>>> pointer but not our fs_info. If it crashes on an unexpected page than
>>> what can we do in the code anyway?
>>
>> What I mean is to check page->mapping for the page passed in.
>>
>> Indeed we can't do anything when we hit a page with NULL mapping
>> pointer, but that's a code bug.
>> An ASSERT() would make us developer aware what's going wrong and to fix
>> the bug.
>
> The assert is a more verbose crash, so that's slightly more developer
> friendly but I'm still not convinced it's worth the assert. Right now
> the macros are not static inlines so they don't need full definitions of
> page and mapping and the other types. Embedding the asserts into macros
> would look like
>
> ({ ASSERT(page); ASSERT(page->mapping); page->mapping->host; })
ASSERT(page) is not needed.
ASSERT(page->mapping) is what I think is necessary.
With something like ({ ASSERT(page); page->mapping->host; }), it still
looks fine to me.
>
> Or perhaps also page with a temporary variable to avoid multiple
> evaluations.
>
> The helpers are used in a handful of places, if we really care about
> consistency of the assertions, something like assert_page_ok(page) would
> have to be in each function that gets the page from other subsystems.
>
The call site that should really be concerned is compression, which
utilize anonymous pages, along with cached pages.
(Scrub is another case, but scrub never mix cached pages with anonymous,
thus it's less a concern)
In fact, during subpage compression development, I have already exposed
several locations where we are trying to pass anonymous page with
manually populated page->i_mapping.
Thus I'm sensitive to this problem.
So far in your code, it's not touching compression code, thus it's fine
for now.
But it's just a problem of time before next refactor to use this macros
for compression code.
Without proper ASSERT(), it's super easy to cause problems IMHO.
Thanks,
Qu
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH 10/10] btrfs: add and use simple page/bio to inode/fs_info helpers
2021-07-27 9:42 ` Qu Wenruo
@ 2021-07-27 14:44 ` David Sterba
0 siblings, 0 replies; 33+ messages in thread
From: David Sterba @ 2021-07-27 14:44 UTC (permalink / raw)
To: Qu Wenruo; +Cc: dsterba, Qu Wenruo, David Sterba, linux-btrfs
On Tue, Jul 27, 2021 at 05:42:10PM +0800, Qu Wenruo wrote:
> > ({ ASSERT(page); ASSERT(page->mapping); page->mapping->host; })
>
> ASSERT(page) is not needed.
>
> ASSERT(page->mapping) is what I think is necessary.
>
> With something like ({ ASSERT(page); page->mapping->host; }), it still
> looks fine to me.
Ok, let's do that.
> > Or perhaps also page with a temporary variable to avoid multiple
> > evaluations.
> >
> > The helpers are used in a handful of places, if we really care about
> > consistency of the assertions, something like assert_page_ok(page) would
> > have to be in each function that gets the page from other subsystems.
> >
>
> The call site that should really be concerned is compression, which
> utilize anonymous pages, along with cached pages.
> (Scrub is another case, but scrub never mix cached pages with anonymous,
> thus it's less a concern)
>
> In fact, during subpage compression development, I have already exposed
> several locations where we are trying to pass anonymous page with
> manually populated page->i_mapping.
>
> Thus I'm sensitive to this problem.
>
> So far in your code, it's not touching compression code, thus it's fine
> for now.
>
> But it's just a problem of time before next refactor to use this macros
> for compression code.
>
> Without proper ASSERT(), it's super easy to cause problems IMHO.
Ok then.
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH v2 10/10] btrfs: add and use simple page/bio to inode/fs_info helpers
2021-07-26 12:15 ` [PATCH 10/10] btrfs: add and use simple page/bio to inode/fs_info helpers David Sterba
2021-07-26 12:41 ` Qu Wenruo
@ 2021-07-27 15:03 ` David Sterba
2021-07-28 0:12 ` Qu Wenruo
1 sibling, 1 reply; 33+ messages in thread
From: David Sterba @ 2021-07-27 15:03 UTC (permalink / raw)
To: linux-btrfs; +Cc: David Sterba, wqu
We have lots of places where we want to obtain inode from page, fs_info
from page and open code the pointer chains.
Add helpers for the most common actions where the types match. There are
still unconverted cases that don't have btrfs_inode and need this
conversion first.
The assertion is supposed to catch unexpected pages without mapping.
Signed-off-by: David Sterba <dsterba@suse.com>
---
v2:
- add page->mapping assertion
fs/btrfs/disk-io.c | 24 +++++++++++++-----------
fs/btrfs/extent_io.c | 33 ++++++++++++++++-----------------
fs/btrfs/inode.c | 4 ++--
fs/btrfs/misc.h | 10 ++++++++++
4 files changed, 41 insertions(+), 30 deletions(-)
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index b117dd3b8172..cdb7791b00d7 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -19,6 +19,7 @@
#include <linux/sched/mm.h>
#include <asm/unaligned.h>
#include <crypto/hash.h>
+#include "misc.h"
#include "ctree.h"
#include "disk-io.h"
#include "transaction.h"
@@ -633,7 +634,7 @@ static int validate_extent_buffer(struct extent_buffer *eb)
static int validate_subpage_buffer(struct page *page, u64 start, u64 end,
int mirror)
{
- struct btrfs_fs_info *fs_info = btrfs_sb(page->mapping->host->i_sb);
+ struct btrfs_fs_info *fs_info = page_to_fs_info(page);
struct extent_buffer *eb;
bool reads_done;
int ret = 0;
@@ -693,7 +694,7 @@ int btrfs_validate_metadata_buffer(struct btrfs_io_bio *io_bio,
ASSERT(page->private);
- if (btrfs_sb(page->mapping->host->i_sb)->sectorsize < PAGE_SIZE)
+ if (page_to_fs_info(page)->sectorsize < PAGE_SIZE)
return validate_subpage_buffer(page, start, end, mirror);
eb = (struct extent_buffer *)page->private;
@@ -876,14 +877,14 @@ blk_status_t btrfs_wq_submit_bio(struct inode *inode, struct bio *bio,
static blk_status_t btree_csum_one_bio(struct bio *bio)
{
struct bio_vec *bvec;
- struct btrfs_root *root;
int ret = 0;
struct bvec_iter_all iter_all;
ASSERT(!bio_flagged(bio, BIO_CLONED));
bio_for_each_segment_all(bvec, bio, iter_all) {
- root = BTRFS_I(bvec->bv_page->mapping->host)->root;
- ret = csum_dirty_buffer(root->fs_info, bvec);
+ struct btrfs_fs_info *fs_info = page_to_fs_info(bvec->bv_page);
+
+ ret = csum_dirty_buffer(fs_info, bvec);
if (ret)
break;
}
@@ -1010,11 +1011,13 @@ static void btree_invalidatepage(struct page *page, unsigned int offset,
unsigned int length)
{
struct extent_io_tree *tree;
- tree = &BTRFS_I(page->mapping->host)->io_tree;
+ struct btrfs_inode *inode = page_to_inode(page);
+
+ tree = &inode->io_tree;
extent_invalidatepage(tree, page, offset);
btree_releasepage(page, GFP_NOFS);
if (PagePrivate(page)) {
- btrfs_warn(BTRFS_I(page->mapping->host)->root->fs_info,
+ btrfs_warn(inode->root->fs_info,
"page private not zero on page %llu",
(unsigned long long)page_offset(page));
detach_page_private(page);
@@ -1024,7 +1027,7 @@ static void btree_invalidatepage(struct page *page, unsigned int offset,
static int btree_set_page_dirty(struct page *page)
{
#ifdef DEBUG
- struct btrfs_fs_info *fs_info = btrfs_sb(page->mapping->host->i_sb);
+ struct btrfs_fs_info *fs_info = page_to_fs_info(page);
struct btrfs_subpage *subpage;
struct extent_buffer *eb;
int cur_bit = 0;
@@ -4437,14 +4440,13 @@ int btrfs_buffer_uptodate(struct extent_buffer *buf, u64 parent_transid,
int atomic)
{
int ret;
- struct inode *btree_inode = buf->pages[0]->mapping->host;
+ struct btrfs_inode *inode = page_to_inode(buf->pages[0]);
ret = extent_buffer_uptodate(buf);
if (!ret)
return ret;
- ret = verify_parent_transid(&BTRFS_I(btree_inode)->io_tree, buf,
- parent_transid, atomic);
+ ret = verify_parent_transid(&inode->io_tree, buf, parent_transid, atomic);
if (ret == -EAGAIN)
return ret;
return !ret;
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 19ba5b03b2df..cb4020ce6419 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -2682,7 +2682,7 @@ int btrfs_repair_one_sector(struct inode *inode,
static void end_page_read(struct page *page, bool uptodate, u64 start, u32 len)
{
- struct btrfs_fs_info *fs_info = btrfs_sb(page->mapping->host->i_sb);
+ struct btrfs_fs_info *fs_info = page_to_fs_info(page);
ASSERT(page_offset(page) <= start &&
start + len <= page_offset(page) + PAGE_SIZE);
@@ -2783,7 +2783,7 @@ void end_extent_writepage(struct page *page, int err, u64 start, u64 end)
int ret = 0;
ASSERT(page && page->mapping);
- inode = BTRFS_I(page->mapping->host);
+ inode = page_to_inode(page);
btrfs_writepage_endio_finish_ordered(inode, page, start, end, uptodate);
if (!uptodate) {
@@ -2815,8 +2815,8 @@ static void end_bio_extent_writepage(struct bio *bio)
ASSERT(!bio_flagged(bio, BIO_CLONED));
bio_for_each_segment_all(bvec, bio, iter_all) {
struct page *page = bvec->bv_page;
- struct inode *inode = page->mapping->host;
- struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
+ struct btrfs_inode *inode = page_to_inode(page);
+ struct btrfs_fs_info *fs_info = inode->root->fs_info;
const u32 sectorsize = fs_info->sectorsize;
/* Our read/write should always be sector aligned. */
@@ -2833,7 +2833,7 @@ static void end_bio_extent_writepage(struct bio *bio)
end = start + bvec->bv_len - 1;
if (first_bvec) {
- btrfs_record_physical_zoned(inode, start, bio);
+ btrfs_record_physical_zoned(&inode->vfs_inode, start, bio);
first_bvec = false;
}
@@ -3306,7 +3306,7 @@ static int submit_extent_page(unsigned int opf,
int ret = 0;
struct bio *bio;
size_t io_size = min_t(size_t, size, PAGE_SIZE);
- struct btrfs_inode *inode = BTRFS_I(page->mapping->host);
+ struct btrfs_inode *inode = page_to_inode(page);
struct extent_io_tree *tree = &inode->io_tree;
struct btrfs_fs_info *fs_info = inode->root->fs_info;
@@ -3334,7 +3334,7 @@ static int submit_extent_page(unsigned int opf,
bio_add_page(bio, page, io_size, pg_offset);
bio->bi_end_io = end_io_func;
bio->bi_private = tree;
- bio->bi_write_hint = page->mapping->host->i_write_hint;
+ bio->bi_write_hint = inode->vfs_inode.i_write_hint;
bio->bi_opf = opf;
if (wbc) {
struct block_device *bdev;
@@ -3410,8 +3410,7 @@ int set_page_extent_mapped(struct page *page)
if (PagePrivate(page))
return 0;
- fs_info = btrfs_sb(page->mapping->host->i_sb);
-
+ fs_info = page_to_fs_info(page);
if (fs_info->sectorsize < PAGE_SIZE)
return btrfs_attach_subpage(fs_info, page, BTRFS_SUBPAGE_DATA);
@@ -3428,7 +3427,7 @@ void clear_page_extent_mapped(struct page *page)
if (!PagePrivate(page))
return;
- fs_info = btrfs_sb(page->mapping->host->i_sb);
+ fs_info = page_to_fs_info(page);
if (fs_info->sectorsize < PAGE_SIZE)
return btrfs_detach_subpage(fs_info, page);
@@ -3670,7 +3669,7 @@ static inline void contiguous_readpages(struct page *pages[], int nr_pages,
struct btrfs_bio_ctrl *bio_ctrl,
u64 *prev_em_start)
{
- struct btrfs_inode *inode = BTRFS_I(pages[0]->mapping->host);
+ struct btrfs_inode *inode = page_to_inode(pages[0]);
int index;
btrfs_lock_and_flush_ordered_range(inode, start, end, NULL);
@@ -4258,7 +4257,7 @@ static void end_bio_subpage_eb_writepage(struct bio *bio)
struct bio_vec *bvec;
struct bvec_iter_all iter_all;
- fs_info = btrfs_sb(bio_first_page_all(bio)->mapping->host->i_sb);
+ fs_info = bio_to_fs_info(bio);
ASSERT(fs_info->sectorsize < PAGE_SIZE);
ASSERT(!bio_flagged(bio, BIO_CLONED));
@@ -4480,7 +4479,7 @@ static int submit_eb_subpage(struct page *page,
struct writeback_control *wbc,
struct extent_page_data *epd)
{
- struct btrfs_fs_info *fs_info = btrfs_sb(page->mapping->host->i_sb);
+ struct btrfs_fs_info *fs_info = page_to_fs_info(page);
int submitted = 0;
u64 page_start = page_offset(page);
int bit_start = 0;
@@ -4586,7 +4585,7 @@ static int submit_eb_page(struct page *page, struct writeback_control *wbc,
if (!PagePrivate(page))
return 0;
- if (btrfs_sb(page->mapping->host->i_sb)->sectorsize < PAGE_SIZE)
+ if (page_to_fs_info(page)->sectorsize < PAGE_SIZE)
return submit_eb_subpage(page, wbc, epd);
spin_lock(&mapping->private_lock);
@@ -5122,7 +5121,7 @@ int try_release_extent_mapping(struct page *page, gfp_t mask)
struct extent_map *em;
u64 start = page_offset(page);
u64 end = start + PAGE_SIZE - 1;
- struct btrfs_inode *btrfs_inode = BTRFS_I(page->mapping->host);
+ struct btrfs_inode *btrfs_inode = page_to_inode(page);
struct extent_io_tree *tree = &btrfs_inode->io_tree;
struct extent_map_tree *map = &btrfs_inode->extent_tree;
@@ -7080,7 +7079,7 @@ static struct extent_buffer *get_next_extent_buffer(
static int try_release_subpage_extent_buffer(struct page *page)
{
- struct btrfs_fs_info *fs_info = btrfs_sb(page->mapping->host->i_sb);
+ struct btrfs_fs_info *fs_info = page_to_fs_info(page);
u64 cur = page_offset(page);
const u64 end = page_offset(page) + PAGE_SIZE;
int ret;
@@ -7152,7 +7151,7 @@ int try_release_extent_buffer(struct page *page)
{
struct extent_buffer *eb;
- if (btrfs_sb(page->mapping->host->i_sb)->sectorsize < PAGE_SIZE)
+ if (page_to_fs_info(page)->sectorsize < PAGE_SIZE)
return try_release_subpage_extent_buffer(page);
/*
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index d31aba370632..159cb2d6a69e 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -8348,7 +8348,7 @@ static int btrfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
int btrfs_readpage(struct file *file, struct page *page)
{
- struct btrfs_inode *inode = BTRFS_I(page->mapping->host);
+ struct btrfs_inode *inode = page_to_inode(page);
u64 start = page_offset(page);
u64 end = start + PAGE_SIZE - 1;
struct btrfs_bio_ctrl bio_ctrl = { 0 };
@@ -8443,7 +8443,7 @@ static int btrfs_migratepage(struct address_space *mapping,
static void btrfs_invalidatepage(struct page *page, unsigned int offset,
unsigned int length)
{
- struct btrfs_inode *inode = BTRFS_I(page->mapping->host);
+ struct btrfs_inode *inode = page_to_inode(page);
struct btrfs_fs_info *fs_info = inode->root->fs_info;
struct extent_io_tree *tree = &inode->io_tree;
struct extent_state *cached_state = NULL;
diff --git a/fs/btrfs/misc.h b/fs/btrfs/misc.h
index 6461ebc3a1c1..9a94ee8dd47b 100644
--- a/fs/btrfs/misc.h
+++ b/fs/btrfs/misc.h
@@ -10,6 +10,16 @@
#define in_range(b, first, len) ((b) >= (first) && (b) < (first) + (len))
+#define page_to_inode(page) \
+({ \
+ struct address_space *page_mapping = (page)->mapping; \
+ ASSERT(page_mapping); \
+ BTRFS_I(page_mapping->host); \
+})
+
+#define page_to_fs_info(page) (page_to_inode(page)->root->fs_info)
+#define bio_to_fs_info(bio) (page_to_fs_info(bio_first_page_all(bio)))
+
static inline void cond_wake_up(struct wait_queue_head *wq)
{
/*
--
2.31.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH v2 10/10] btrfs: add and use simple page/bio to inode/fs_info helpers
2021-07-27 15:03 ` [PATCH v2 " David Sterba
@ 2021-07-28 0:12 ` Qu Wenruo
0 siblings, 0 replies; 33+ messages in thread
From: Qu Wenruo @ 2021-07-28 0:12 UTC (permalink / raw)
To: David Sterba, linux-btrfs; +Cc: wqu
On 2021/7/27 下午11:03, David Sterba wrote:
> We have lots of places where we want to obtain inode from page, fs_info
> from page and open code the pointer chains.
>
> Add helpers for the most common actions where the types match. There are
> still unconverted cases that don't have btrfs_inode and need this
> conversion first.
>
> The assertion is supposed to catch unexpected pages without mapping.
>
> Signed-off-by: David Sterba <dsterba@suse.com>
Reviewed-by: Qu Wenruo <wqu@suse.com>
Thanks,
Qu
> ---
>
> v2:
>
> - add page->mapping assertion
>
> fs/btrfs/disk-io.c | 24 +++++++++++++-----------
> fs/btrfs/extent_io.c | 33 ++++++++++++++++-----------------
> fs/btrfs/inode.c | 4 ++--
> fs/btrfs/misc.h | 10 ++++++++++
> 4 files changed, 41 insertions(+), 30 deletions(-)
>
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index b117dd3b8172..cdb7791b00d7 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -19,6 +19,7 @@
> #include <linux/sched/mm.h>
> #include <asm/unaligned.h>
> #include <crypto/hash.h>
> +#include "misc.h"
> #include "ctree.h"
> #include "disk-io.h"
> #include "transaction.h"
> @@ -633,7 +634,7 @@ static int validate_extent_buffer(struct extent_buffer *eb)
> static int validate_subpage_buffer(struct page *page, u64 start, u64 end,
> int mirror)
> {
> - struct btrfs_fs_info *fs_info = btrfs_sb(page->mapping->host->i_sb);
> + struct btrfs_fs_info *fs_info = page_to_fs_info(page);
> struct extent_buffer *eb;
> bool reads_done;
> int ret = 0;
> @@ -693,7 +694,7 @@ int btrfs_validate_metadata_buffer(struct btrfs_io_bio *io_bio,
>
> ASSERT(page->private);
>
> - if (btrfs_sb(page->mapping->host->i_sb)->sectorsize < PAGE_SIZE)
> + if (page_to_fs_info(page)->sectorsize < PAGE_SIZE)
> return validate_subpage_buffer(page, start, end, mirror);
>
> eb = (struct extent_buffer *)page->private;
> @@ -876,14 +877,14 @@ blk_status_t btrfs_wq_submit_bio(struct inode *inode, struct bio *bio,
> static blk_status_t btree_csum_one_bio(struct bio *bio)
> {
> struct bio_vec *bvec;
> - struct btrfs_root *root;
> int ret = 0;
> struct bvec_iter_all iter_all;
>
> ASSERT(!bio_flagged(bio, BIO_CLONED));
> bio_for_each_segment_all(bvec, bio, iter_all) {
> - root = BTRFS_I(bvec->bv_page->mapping->host)->root;
> - ret = csum_dirty_buffer(root->fs_info, bvec);
> + struct btrfs_fs_info *fs_info = page_to_fs_info(bvec->bv_page);
> +
> + ret = csum_dirty_buffer(fs_info, bvec);
> if (ret)
> break;
> }
> @@ -1010,11 +1011,13 @@ static void btree_invalidatepage(struct page *page, unsigned int offset,
> unsigned int length)
> {
> struct extent_io_tree *tree;
> - tree = &BTRFS_I(page->mapping->host)->io_tree;
> + struct btrfs_inode *inode = page_to_inode(page);
> +
> + tree = &inode->io_tree;
> extent_invalidatepage(tree, page, offset);
> btree_releasepage(page, GFP_NOFS);
> if (PagePrivate(page)) {
> - btrfs_warn(BTRFS_I(page->mapping->host)->root->fs_info,
> + btrfs_warn(inode->root->fs_info,
> "page private not zero on page %llu",
> (unsigned long long)page_offset(page));
> detach_page_private(page);
> @@ -1024,7 +1027,7 @@ static void btree_invalidatepage(struct page *page, unsigned int offset,
> static int btree_set_page_dirty(struct page *page)
> {
> #ifdef DEBUG
> - struct btrfs_fs_info *fs_info = btrfs_sb(page->mapping->host->i_sb);
> + struct btrfs_fs_info *fs_info = page_to_fs_info(page);
> struct btrfs_subpage *subpage;
> struct extent_buffer *eb;
> int cur_bit = 0;
> @@ -4437,14 +4440,13 @@ int btrfs_buffer_uptodate(struct extent_buffer *buf, u64 parent_transid,
> int atomic)
> {
> int ret;
> - struct inode *btree_inode = buf->pages[0]->mapping->host;
> + struct btrfs_inode *inode = page_to_inode(buf->pages[0]);
>
> ret = extent_buffer_uptodate(buf);
> if (!ret)
> return ret;
>
> - ret = verify_parent_transid(&BTRFS_I(btree_inode)->io_tree, buf,
> - parent_transid, atomic);
> + ret = verify_parent_transid(&inode->io_tree, buf, parent_transid, atomic);
> if (ret == -EAGAIN)
> return ret;
> return !ret;
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 19ba5b03b2df..cb4020ce6419 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -2682,7 +2682,7 @@ int btrfs_repair_one_sector(struct inode *inode,
>
> static void end_page_read(struct page *page, bool uptodate, u64 start, u32 len)
> {
> - struct btrfs_fs_info *fs_info = btrfs_sb(page->mapping->host->i_sb);
> + struct btrfs_fs_info *fs_info = page_to_fs_info(page);
>
> ASSERT(page_offset(page) <= start &&
> start + len <= page_offset(page) + PAGE_SIZE);
> @@ -2783,7 +2783,7 @@ void end_extent_writepage(struct page *page, int err, u64 start, u64 end)
> int ret = 0;
>
> ASSERT(page && page->mapping);
> - inode = BTRFS_I(page->mapping->host);
> + inode = page_to_inode(page);
> btrfs_writepage_endio_finish_ordered(inode, page, start, end, uptodate);
>
> if (!uptodate) {
> @@ -2815,8 +2815,8 @@ static void end_bio_extent_writepage(struct bio *bio)
> ASSERT(!bio_flagged(bio, BIO_CLONED));
> bio_for_each_segment_all(bvec, bio, iter_all) {
> struct page *page = bvec->bv_page;
> - struct inode *inode = page->mapping->host;
> - struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
> + struct btrfs_inode *inode = page_to_inode(page);
> + struct btrfs_fs_info *fs_info = inode->root->fs_info;
> const u32 sectorsize = fs_info->sectorsize;
>
> /* Our read/write should always be sector aligned. */
> @@ -2833,7 +2833,7 @@ static void end_bio_extent_writepage(struct bio *bio)
> end = start + bvec->bv_len - 1;
>
> if (first_bvec) {
> - btrfs_record_physical_zoned(inode, start, bio);
> + btrfs_record_physical_zoned(&inode->vfs_inode, start, bio);
> first_bvec = false;
> }
>
> @@ -3306,7 +3306,7 @@ static int submit_extent_page(unsigned int opf,
> int ret = 0;
> struct bio *bio;
> size_t io_size = min_t(size_t, size, PAGE_SIZE);
> - struct btrfs_inode *inode = BTRFS_I(page->mapping->host);
> + struct btrfs_inode *inode = page_to_inode(page);
> struct extent_io_tree *tree = &inode->io_tree;
> struct btrfs_fs_info *fs_info = inode->root->fs_info;
>
> @@ -3334,7 +3334,7 @@ static int submit_extent_page(unsigned int opf,
> bio_add_page(bio, page, io_size, pg_offset);
> bio->bi_end_io = end_io_func;
> bio->bi_private = tree;
> - bio->bi_write_hint = page->mapping->host->i_write_hint;
> + bio->bi_write_hint = inode->vfs_inode.i_write_hint;
> bio->bi_opf = opf;
> if (wbc) {
> struct block_device *bdev;
> @@ -3410,8 +3410,7 @@ int set_page_extent_mapped(struct page *page)
> if (PagePrivate(page))
> return 0;
>
> - fs_info = btrfs_sb(page->mapping->host->i_sb);
> -
> + fs_info = page_to_fs_info(page);
> if (fs_info->sectorsize < PAGE_SIZE)
> return btrfs_attach_subpage(fs_info, page, BTRFS_SUBPAGE_DATA);
>
> @@ -3428,7 +3427,7 @@ void clear_page_extent_mapped(struct page *page)
> if (!PagePrivate(page))
> return;
>
> - fs_info = btrfs_sb(page->mapping->host->i_sb);
> + fs_info = page_to_fs_info(page);
> if (fs_info->sectorsize < PAGE_SIZE)
> return btrfs_detach_subpage(fs_info, page);
>
> @@ -3670,7 +3669,7 @@ static inline void contiguous_readpages(struct page *pages[], int nr_pages,
> struct btrfs_bio_ctrl *bio_ctrl,
> u64 *prev_em_start)
> {
> - struct btrfs_inode *inode = BTRFS_I(pages[0]->mapping->host);
> + struct btrfs_inode *inode = page_to_inode(pages[0]);
> int index;
>
> btrfs_lock_and_flush_ordered_range(inode, start, end, NULL);
> @@ -4258,7 +4257,7 @@ static void end_bio_subpage_eb_writepage(struct bio *bio)
> struct bio_vec *bvec;
> struct bvec_iter_all iter_all;
>
> - fs_info = btrfs_sb(bio_first_page_all(bio)->mapping->host->i_sb);
> + fs_info = bio_to_fs_info(bio);
> ASSERT(fs_info->sectorsize < PAGE_SIZE);
>
> ASSERT(!bio_flagged(bio, BIO_CLONED));
> @@ -4480,7 +4479,7 @@ static int submit_eb_subpage(struct page *page,
> struct writeback_control *wbc,
> struct extent_page_data *epd)
> {
> - struct btrfs_fs_info *fs_info = btrfs_sb(page->mapping->host->i_sb);
> + struct btrfs_fs_info *fs_info = page_to_fs_info(page);
> int submitted = 0;
> u64 page_start = page_offset(page);
> int bit_start = 0;
> @@ -4586,7 +4585,7 @@ static int submit_eb_page(struct page *page, struct writeback_control *wbc,
> if (!PagePrivate(page))
> return 0;
>
> - if (btrfs_sb(page->mapping->host->i_sb)->sectorsize < PAGE_SIZE)
> + if (page_to_fs_info(page)->sectorsize < PAGE_SIZE)
> return submit_eb_subpage(page, wbc, epd);
>
> spin_lock(&mapping->private_lock);
> @@ -5122,7 +5121,7 @@ int try_release_extent_mapping(struct page *page, gfp_t mask)
> struct extent_map *em;
> u64 start = page_offset(page);
> u64 end = start + PAGE_SIZE - 1;
> - struct btrfs_inode *btrfs_inode = BTRFS_I(page->mapping->host);
> + struct btrfs_inode *btrfs_inode = page_to_inode(page);
> struct extent_io_tree *tree = &btrfs_inode->io_tree;
> struct extent_map_tree *map = &btrfs_inode->extent_tree;
>
> @@ -7080,7 +7079,7 @@ static struct extent_buffer *get_next_extent_buffer(
>
> static int try_release_subpage_extent_buffer(struct page *page)
> {
> - struct btrfs_fs_info *fs_info = btrfs_sb(page->mapping->host->i_sb);
> + struct btrfs_fs_info *fs_info = page_to_fs_info(page);
> u64 cur = page_offset(page);
> const u64 end = page_offset(page) + PAGE_SIZE;
> int ret;
> @@ -7152,7 +7151,7 @@ int try_release_extent_buffer(struct page *page)
> {
> struct extent_buffer *eb;
>
> - if (btrfs_sb(page->mapping->host->i_sb)->sectorsize < PAGE_SIZE)
> + if (page_to_fs_info(page)->sectorsize < PAGE_SIZE)
> return try_release_subpage_extent_buffer(page);
>
> /*
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index d31aba370632..159cb2d6a69e 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -8348,7 +8348,7 @@ static int btrfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
>
> int btrfs_readpage(struct file *file, struct page *page)
> {
> - struct btrfs_inode *inode = BTRFS_I(page->mapping->host);
> + struct btrfs_inode *inode = page_to_inode(page);
> u64 start = page_offset(page);
> u64 end = start + PAGE_SIZE - 1;
> struct btrfs_bio_ctrl bio_ctrl = { 0 };
> @@ -8443,7 +8443,7 @@ static int btrfs_migratepage(struct address_space *mapping,
> static void btrfs_invalidatepage(struct page *page, unsigned int offset,
> unsigned int length)
> {
> - struct btrfs_inode *inode = BTRFS_I(page->mapping->host);
> + struct btrfs_inode *inode = page_to_inode(page);
> struct btrfs_fs_info *fs_info = inode->root->fs_info;
> struct extent_io_tree *tree = &inode->io_tree;
> struct extent_state *cached_state = NULL;
> diff --git a/fs/btrfs/misc.h b/fs/btrfs/misc.h
> index 6461ebc3a1c1..9a94ee8dd47b 100644
> --- a/fs/btrfs/misc.h
> +++ b/fs/btrfs/misc.h
> @@ -10,6 +10,16 @@
>
> #define in_range(b, first, len) ((b) >= (first) && (b) < (first) + (len))
>
> +#define page_to_inode(page) \
> +({ \
> + struct address_space *page_mapping = (page)->mapping; \
> + ASSERT(page_mapping); \
> + BTRFS_I(page_mapping->host); \
> +})
> +
> +#define page_to_fs_info(page) (page_to_inode(page)->root->fs_info)
> +#define bio_to_fs_info(bio) (page_to_fs_info(bio_first_page_all(bio)))
> +
> static inline void cond_wake_up(struct wait_queue_head *wq)
> {
> /*
>
^ permalink raw reply [flat|nested] 33+ messages in thread