[PATCH v2 0/3] Misc cosmetic changes for map_private_extent_buffer

[PATCH v2 0/3] Misc cosmetic changes for map_private_extent_buffer

[Johannes Thumshirn]

While trying to understand the checksum code I came across some oddities
regarding map_private_extent_buffer() and it's interaction with
csum_tree_block().

These patches address them but are either purely cosmetic or only add a
comment documenting behaviour.

Changes to v1:
* Add Reviewed-by tags
* Change wording of commit message in patch 3/3

Johannes Thumshirn (3):
  btrfs: don't initialize 'offset' in map_private_extent_buffer()
  btrfs: use offset_in_page for start_offset in
    map_private_extent_buffer()
  btrfs: document extent mapping assumptions in checksum

 fs/btrfs/disk-io.c   | 6 ++++++
 fs/btrfs/extent_io.c | 4 ++--
 2 files changed, 8 insertions(+), 2 deletions(-)

-- 
2.16.4

[PATCH v2 1/3] btrfs: don't initialize 'offset' in map_private_extent_buffer()

[Johannes Thumshirn]

In map_private_extent_buffer() the 'offset' variable is initialized to a
page aligned version of the 'start' parameter.

But later on it is overwritten with either the offset from the extent
buffer's start or 0.

So get rid of the initial initialization.

Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/extent_io.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 582b4b1c41e0..7aafdec49dc3 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -5380,7 +5380,7 @@ int map_private_extent_buffer(const struct extent_buffer *eb,
 			      char **map, unsigned long *map_start,
 			      unsigned long *map_len)
 {
-	size_t offset = start & (PAGE_SIZE - 1);
+	size_t offset;
 	char *kaddr;
 	struct page *p;
 	size_t start_offset = eb->start & ((u64)PAGE_SIZE - 1);
-- 
2.16.4

[PATCH v2 2/3] btrfs: use offset_in_page for start_offset in map_private_extent_buffer()

[Johannes Thumshirn]

In map_private_extent_buffer() use offset_in_page() to initialize
'start_offset' instead of open-coding it.

Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/extent_io.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 7aafdec49dc3..85cd3975c680 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -5383,7 +5383,7 @@ int map_private_extent_buffer(const struct extent_buffer *eb,
 	size_t offset;
 	char *kaddr;
 	struct page *p;
-	size_t start_offset = eb->start & ((u64)PAGE_SIZE - 1);
+	size_t start_offset = offset_in_page(eb->start);
 	unsigned long i = (start_offset + start) >> PAGE_SHIFT;
 	unsigned long end_i = (start_offset + start + min_len - 1) >>
 		PAGE_SHIFT;
-- 
2.16.4

[PATCH v2 3/3] btrfs: document extent mapping assumptions in checksum

[Johannes Thumshirn]

Document why map_private_extent_buffer() cannot return '1' (i.e. the map
spans two pages) for the csum_tree_block() case.

The current algorithm for detecting a page boundary crossing in
map_private_extent_buffer() will return a '1' *IFF* the extent buffer's
offset in the page + the offset passed in by csum_tree_block() and the
minimal length passed in by csum_tree_block() - 1 are bigger than
PAGE_SIZE.

We always pass BTRFS_CSUM_SIZE (32) as offset and a minimal length of 32
and the current extent buffer allocator always guarantees page aligned
extends, so the above condition can't be true.

Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>

---
Changes to v1:
* Changed wording of the commit message according to Noah's suggestion
---
 fs/btrfs/disk-io.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 4bc270ef29b4..14d355d0cb7a 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -279,6 +279,12 @@ static int csum_tree_block(struct btrfs_fs_info *fs_info,
 
 	len = buf->len - offset;
 	while (len > 0) {
+		/*
+		 * Note: we don't need to check for the err == 1 case here, as
+		 * with the given combination of 'start = BTRFS_CSUM_SIZE (32)'
+		 * and 'min_len = 32' and the currently implemented mapping
+		 * algorithm we cannot cross a page boundary.
+		 */
 		err = map_private_extent_buffer(buf, offset, 32,
 					&kaddr, &map_start, &map_len);
 		if (err)
-- 
2.16.4

Re: [PATCH v2 3/3] btrfs: document extent mapping assumptions in checksum

[Nikolay Borisov]

On 28.11.18 г. 10:54 ч., Johannes Thumshirn wrote:
> Document why map_private_extent_buffer() cannot return '1' (i.e. the map
> spans two pages) for the csum_tree_block() case.
> 
> The current algorithm for detecting a page boundary crossing in
> map_private_extent_buffer() will return a '1' *IFF* the extent buffer's
> offset in the page + the offset passed in by csum_tree_block() and the
> minimal length passed in by csum_tree_block() - 1 are bigger than
> PAGE_SIZE.
> 
> We always pass BTRFS_CSUM_SIZE (32) as offset and a minimal length of 32
> and the current extent buffer allocator always guarantees page aligned
> extends, so the above condition can't be true.
> 
> Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>

Reviewed-by: Nikolay Borisov <nborisov@suse.com>


> 
> ---
> Changes to v1:
> * Changed wording of the commit message according to Noah's suggestion
> ---
>  fs/btrfs/disk-io.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 4bc270ef29b4..14d355d0cb7a 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -279,6 +279,12 @@ static int csum_tree_block(struct btrfs_fs_info *fs_info,
>  
>  	len = buf->len - offset;
>  	while (len > 0) {
> +		/*
> +		 * Note: we don't need to check for the err == 1 case here, as
> +		 * with the given combination of 'start = BTRFS_CSUM_SIZE (32)'
> +		 * and 'min_len = 32' and the currently implemented mapping
> +		 * algorithm we cannot cross a page boundary.
> +		 */
>  		err = map_private_extent_buffer(buf, offset, 32,
>  					&kaddr, &map_start, &map_len);
>  		if (err)
> 

Re: [PATCH v2 2/3] btrfs: use offset_in_page for start_offset in map_private_extent_buffer()

[David Sterba]

On Wed, Nov 28, 2018 at 09:54:55AM +0100, Johannes Thumshirn wrote:
> In map_private_extent_buffer() use offset_in_page() to initialize
> 'start_offset' instead of open-coding it.

Can you please fix all instances where it's opencoded? Grepping for
'PAGE_SIZE - 1' finds a number of them. Thanks.

Re: [PATCH v2 0/3] Misc cosmetic changes for map_private_extent_buffer

[David Sterba]

On Wed, Nov 28, 2018 at 09:54:53AM +0100, Johannes Thumshirn wrote:
> While trying to understand the checksum code I came across some oddities
> regarding map_private_extent_buffer() and it's interaction with
> csum_tree_block().
> 
> These patches address them but are either purely cosmetic or only add a
> comment documenting behaviour.
> 
> Changes to v1:
> * Add Reviewed-by tags
> * Change wording of commit message in patch 3/3
> 
> Johannes Thumshirn (3):
>   btrfs: don't initialize 'offset' in map_private_extent_buffer()
>   btrfs: use offset_in_page for start_offset in
>     map_private_extent_buffer()
>   btrfs: document extent mapping assumptions in checksum

1 and 3 applied, thanks.

Re: [PATCH v2 2/3] btrfs: use offset_in_page for start_offset in map_private_extent_buffer()

[Johannes Thumshirn]

On 28/11/2018 16:41, David Sterba wrote:
> On Wed, Nov 28, 2018 at 09:54:55AM +0100, Johannes Thumshirn wrote:
>> In map_private_extent_buffer() use offset_in_page() to initialize
>> 'start_offset' instead of open-coding it.
> 
> Can you please fix all instances where it's opencoded? Grepping for
> 'PAGE_SIZE - 1' finds a number of them. Thanks.

Sure will do.

-- 
Johannes Thumshirn                            SUSE Labs Filesystems
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850