[PATCH 0/3] Misc cosmetic changes for map_private_extent_buffer

[PATCH 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.

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 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>
---
 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 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>
---
 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 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 product of 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>
---
 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 1/3] btrfs: don't initialize 'offset' in map_private_extent_buffer()

[Nikolay Borisov]

On 27.11.18 г. 18:00 ч., Johannes Thumshirn wrote:
> 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>

You know, the fastest/most clean code is the one which is deleted so :

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);
> 

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

[Nikolay Borisov]

On 27.11.18 г. 18:00 ч., Johannes Thumshirn wrote:
> 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;
> 

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

[Nikolay Borisov]

On 27.11.18 г. 18:00 ч., 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 product of the

I think the word product must be replaced with 'sum', since product
implies multiplication :)

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

With that wording changed:

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

> ---
>  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 3/3] btrfs: document extent mapping assumptions in checksum

[Noah Massey]

On Tue, Nov 27, 2018 at 11:43 AM Nikolay Borisov <nborisov@suse.com> wrote:
>
> On 27.11.18 г. 18:00 ч., 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 product of the
>
> I think the word product must be replaced with 'sum', since product
> implies multiplication :)
>

doesn't 'sum' imply addition? How about 'output'?

> > 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>
>
> With that wording changed:
>
> Reviewed-by: Nikolay Borisov <nborisov@suse.com>
>
> > ---
> >  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 3/3] btrfs: document extent mapping assumptions in checksum

[Nikolay Borisov]

On 27.11.18 г. 21:08 ч., Noah Massey wrote:
> On Tue, Nov 27, 2018 at 11:43 AM Nikolay Borisov <nborisov@suse.com> wrote:
>>
>> On 27.11.18 г. 18:00 ч., 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 product of the
>>
>> I think the word product must be replaced with 'sum', since product
>> implies multiplication :)
>>
> 
> doesn't 'sum' imply addition? How about 'output'?

It does and the code indeed sums the value and not multiply them hence
my suggestion.

> 
>>> 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>
>>
>> With that wording changed:
>>
>> Reviewed-by: Nikolay Borisov <nborisov@suse.com>
>>
>>> ---
>>>  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 3/3] btrfs: document extent mapping assumptions in checksum

[Noah Massey]

On Tue, Nov 27, 2018 at 2:32 PM Nikolay Borisov <nborisov@suse.com> wrote:
>
> On 27.11.18 г. 21:08 ч., Noah Massey wrote:
> > On Tue, Nov 27, 2018 at 11:43 AM Nikolay Borisov <nborisov@suse.com> wrote:
> >>
> >> On 27.11.18 г. 18:00 ч., 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 product of the
> >>
> >> I think the word product must be replaced with 'sum', since product
> >> implies multiplication :)
> >>
> >
> > doesn't 'sum' imply addition? How about 'output'?
>
> It does and the code indeed sums the value and not multiply them hence
> my suggestion.
>

I'm sorry, I didn't phrase that well.

Since 'sum' already implies addition, it gets confusing with the
mathematical operators used later in the description. So, if a
objective noun is required, a generic term such as 'output' or
'result' reads more cleanly for me. OTOH, dropping that and creating
an actual expression

*IFF* the extent buffer's offset in the page + the offset passed in by
csum_tree_block() + the minimal length passed in by csum_tree_block()
- 1 > PAGE_SIZE.

is also straightforward.

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

[Johannes Thumshirn]

On 27/11/2018 17:36, Nikolay Borisov wrote:
> 
> 
> On 27.11.18 г. 18:00 ч., 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 product of the
> 
> I think the word product must be replaced with 'sum', since product
> implies multiplication :)

*doh* m)

Yes thanks for spotting this.

-- 
Johannes Thumshirn                                        SUSE Labs
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