Re: [PATCH v5 6/7] libnvdimm, btt: refactor initial_offset calculations

[Dan Williams]

On Mon, Aug 14, 2017 at 12:47 PM, Verma, Vishal L
 wrote:
> On Fri, 2017-08-11 at 18:39 -0700, Dan Williams wrote:
>> On Tue, Aug 8, 2017 at 5:07 PM, Vishal Verma > > wrote:
>> > In preparation for BTT error clearing, refactor the initial offset
>> > calculations. Until now, all callers of arena_{read,write}_bytes
>> > assumed
>> > a relative offset to the arena, and it was later adjusted for the
>> > initial offset. Until now, every time we calculated a relative
>> > offset,
>> > we passed it to these functions to do reads or writes, and so where
>> > the
>> > offset calculations happened didn't matter.
>> >
>> > For error clearing, there will be places where we calculate offsets
>> > to
>> > check for the presence of media errors, and the above scheme becomes
>> > error prone.
>> >
>> > Make the initial_offset calculations explicit for all callers of
>> > arena_{read,write}_bytes, and provide a helper to do them. The error
>> > checking/clearing code can then use these same helpers.
>> >
>> > Reported-by: Toshi Kani 
>> > Signed-off-by: Vishal Verma 
>> > ---
>> >  drivers/nvdimm/btt.c | 69 -
>> > ---
>> >  1 file changed, 42 insertions(+), 27 deletions(-)
>> >
>> > diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c
>> > index e9dd651..9acf06b 100644
>> > --- a/drivers/nvdimm/btt.c
>> > +++ b/drivers/nvdimm/btt.c
>> > @@ -31,14 +31,17 @@ enum log_ent_request {
>> > LOG_OLD_ENT
>> >  };
>> >
>> > +static inline u64 calc_nsoff(struct nd_btt *nd_btt, u64
>> > relative_offset)
>> > +{
>> > +   return relative_offset + nd_btt->initial_offset;
>> > +}
>> > +
>> >  static int arena_read_bytes(struct arena_info *arena,
>> > resource_size_t offset,
>> > void *buf, size_t n, unsigned long flags)
>> >  {
>> > struct nd_btt *nd_btt = arena->nd_btt;
>> > struct nd_namespace_common *ndns = nd_btt->ndns;
>> >
>> > -   /* arena offsets may be shifted from the base of the device
>> > */
>> > -   offset += arena->nd_btt->initial_offset;
>> > return nvdimm_read_bytes(ndns, offset, buf, n, flags);
>> >  }
>> >
>> > @@ -48,14 +51,13 @@ static int arena_write_bytes(struct arena_info
>> > *arena, resource_size_t offset,
>> > struct nd_btt *nd_btt = arena->nd_btt;
>> > struct nd_namespace_common *ndns = nd_btt->ndns;
>> >
>> > -   /* arena offsets may be shifted from the base of the device
>> > */
>> > -   offset += arena->nd_btt->initial_offset;
>> > return nvdimm_write_bytes(ndns, offset, buf, n, flags);
>> >  }
>>
>> So let's get rid of arena_{read,write}_bytes(). The only point of the
>> wrapper was to include an arena specific offset, but now we're pushing
>> all offset calculations to the caller so arena_{read,write}_bytes() is
>> equivalent to nvdimm_{read,write}_bytes().
>
> That sounds fine, though that will require all callers to now
> dereference arena->nd->btt->ndns to do reads/writes.. Is that ok?

Sure, just put that in a to_ndns() helper.

>> However, since many of
>> these call sites now perform a common offset calculations maybe we can
>> provide a new helper for that to differentiate it from the error
>> clearing case?
>
> Not sure I follow - the offset calculations for the error clearing use
> and for data reads/writes are exactly the same (lba_to_nsoff). (See
> further below)
>
>> After patch 7 is applied it's no longer clear which
>> offset calculation is used where and for what reason, there's just too
>> many offset variables calculations and overload of the word 'offset'
>> for bugs to hide.
>
> Agreed on the ambiguity of 'offset'. The implicit rule I followed was
> any kind of 'nsoff' will mean the final namespace offset, after
> accounting for arenas, any initial offset etc. Any other 'offset' is
> relative to the context in which it is used.
>
>>
>> Maybe it's just late on a Friday and my eyes are going cross, but it
>> seems you have 2 classes of use cases between calc_nsoff and
>> lba_to_nsoff, perhaps separate read/write wrappers for those 2 cases?
>
> The difference between the two is that lba_to_nsoff is used when were
> talking about reading/writing a 'data' block, and where we are talking
> in terms of 'lba's, not raw offsets. calc_nsoff (which lba_to_nsoff
> calls internally), is a 'last stage' calculation, essentially just to
> add the initial_offset. Direct callers of calc_nsoff (i.e. functions
> that access the log, map, or info blocks) do their offset calculations
> 'manually' based on arena->{map,log,info}off, but these offsets are
> unaware of 'initial_offset', as they started out being relative to
> 'this' arena. So calc_nsoff is that last stage addition of
> initial_offset.
>
> I agree that if feels a bit clunky, but I'm not sure I see a clear way
> to improve it..

Since calc_nsoff is "last stage" and required I think it should be
hidden in 

Re: [PATCH v5 6/7] libnvdimm, btt: refactor initial_offset calculations

[Verma, Vishal L]

On Fri, 2017-08-11 at 18:39 -0700, Dan Williams wrote:
> On Tue, Aug 8, 2017 at 5:07 PM, Vishal Verma  > wrote:
> > In preparation for BTT error clearing, refactor the initial offset
> > calculations. Until now, all callers of arena_{read,write}_bytes
> > assumed
> > a relative offset to the arena, and it was later adjusted for the
> > initial offset. Until now, every time we calculated a relative
> > offset,
> > we passed it to these functions to do reads or writes, and so where
> > the
> > offset calculations happened didn't matter.
> > 
> > For error clearing, there will be places where we calculate offsets
> > to
> > check for the presence of media errors, and the above scheme becomes
> > error prone.
> > 
> > Make the initial_offset calculations explicit for all callers of
> > arena_{read,write}_bytes, and provide a helper to do them. The error
> > checking/clearing code can then use these same helpers.
> > 
> > Reported-by: Toshi Kani 
> > Signed-off-by: Vishal Verma 
> > ---
> >  drivers/nvdimm/btt.c | 69 -
> > ---
> >  1 file changed, 42 insertions(+), 27 deletions(-)
> > 
> > diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c
> > index e9dd651..9acf06b 100644
> > --- a/drivers/nvdimm/btt.c
> > +++ b/drivers/nvdimm/btt.c
> > @@ -31,14 +31,17 @@ enum log_ent_request {
> > LOG_OLD_ENT
> >  };
> > 
> > +static inline u64 calc_nsoff(struct nd_btt *nd_btt, u64
> > relative_offset)
> > +{
> > +   return relative_offset + nd_btt->initial_offset;
> > +}
> > +
> >  static int arena_read_bytes(struct arena_info *arena,
> > resource_size_t offset,
> > void *buf, size_t n, unsigned long flags)
> >  {
> > struct nd_btt *nd_btt = arena->nd_btt;
> > struct nd_namespace_common *ndns = nd_btt->ndns;
> > 
> > -   /* arena offsets may be shifted from the base of the device
> > */
> > -   offset += arena->nd_btt->initial_offset;
> > return nvdimm_read_bytes(ndns, offset, buf, n, flags);
> >  }
> > 
> > @@ -48,14 +51,13 @@ static int arena_write_bytes(struct arena_info
> > *arena, resource_size_t offset,
> > struct nd_btt *nd_btt = arena->nd_btt;
> > struct nd_namespace_common *ndns = nd_btt->ndns;
> > 
> > -   /* arena offsets may be shifted from the base of the device
> > */
> > -   offset += arena->nd_btt->initial_offset;
> > return nvdimm_write_bytes(ndns, offset, buf, n, flags);
> >  }
> 
> So let's get rid of arena_{read,write}_bytes(). The only point of the
> wrapper was to include an arena specific offset, but now we're pushing
> all offset calculations to the caller so arena_{read,write}_bytes() is
> equivalent to nvdimm_{read,write}_bytes(). 

That sounds fine, though that will require all callers to now
dereference arena->nd->btt->ndns to do reads/writes.. Is that ok?

> However, since many of
> these call sites now perform a common offset calculations maybe we can
> provide a new helper for that to differentiate it from the error
> clearing case? 

Not sure I follow - the offset calculations for the error clearing use
and for data reads/writes are exactly the same (lba_to_nsoff). (See
further below)

> After patch 7 is applied it's no longer clear which
> offset calculation is used where and for what reason, there's just too
> many offset variables calculations and overload of the word 'offset'
> for bugs to hide.

Agreed on the ambiguity of 'offset'. The implicit rule I followed was
any kind of 'nsoff' will mean the final namespace offset, after
accounting for arenas, any initial offset etc. Any other 'offset' is
relative to the context in which it is used.

> 
> Maybe it's just late on a Friday and my eyes are going cross, but it
> seems you have 2 classes of use cases between calc_nsoff and
> lba_to_nsoff, perhaps separate read/write wrappers for those 2 cases?

The difference between the two is that lba_to_nsoff is used when were
talking about reading/writing a 'data' block, and where we are talking
in terms of 'lba's, not raw offsets. calc_nsoff (which lba_to_nsoff
calls internally), is a 'last stage' calculation, essentially just to
add the initial_offset. Direct callers of calc_nsoff (i.e. functions
that access the log, map, or info blocks) do their offset calculations
'manually' based on arena->{map,log,info}off, but these offsets are
unaware of 'initial_offset', as they started out being relative to
'this' arena. So calc_nsoff is that last stage addition of
initial_offset.

I agree that if feels a bit clunky, but I'm not sure I see a clear way
to improve it..

> Also, I find something unreadable about calc_nsoff, and reading the
> prototype "u64 calc_nsoff(struct nd_btt *nd_btt, u64 relative_offset)"
> doesn't help clarify. What is 'relative_offset' relative to? Is it the
> base namespace / device offset?

Yeah, I cringed a bit too when I called it 'relative_offset'. But I
couldn't 

Re: [PATCH v5 6/7] libnvdimm, btt: refactor initial_offset calculations

[Dan Williams]

On Tue, Aug 8, 2017 at 5:07 PM, Vishal Verma  wrote:
> In preparation for BTT error clearing, refactor the initial offset
> calculations. Until now, all callers of arena_{read,write}_bytes assumed
> a relative offset to the arena, and it was later adjusted for the
> initial offset. Until now, every time we calculated a relative offset,
> we passed it to these functions to do reads or writes, and so where the
> offset calculations happened didn't matter.
>
> For error clearing, there will be places where we calculate offsets to
> check for the presence of media errors, and the above scheme becomes
> error prone.
>
> Make the initial_offset calculations explicit for all callers of
> arena_{read,write}_bytes, and provide a helper to do them. The error
> checking/clearing code can then use these same helpers.
>
> Reported-by: Toshi Kani 
> Signed-off-by: Vishal Verma 
> ---
>  drivers/nvdimm/btt.c | 69 
> 
>  1 file changed, 42 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c
> index e9dd651..9acf06b 100644
> --- a/drivers/nvdimm/btt.c
> +++ b/drivers/nvdimm/btt.c
> @@ -31,14 +31,17 @@ enum log_ent_request {
> LOG_OLD_ENT
>  };
>
> +static inline u64 calc_nsoff(struct nd_btt *nd_btt, u64 relative_offset)
> +{
> +   return relative_offset + nd_btt->initial_offset;
> +}
> +
>  static int arena_read_bytes(struct arena_info *arena, resource_size_t offset,
> void *buf, size_t n, unsigned long flags)
>  {
> struct nd_btt *nd_btt = arena->nd_btt;
> struct nd_namespace_common *ndns = nd_btt->ndns;
>
> -   /* arena offsets may be shifted from the base of the device */
> -   offset += arena->nd_btt->initial_offset;
> return nvdimm_read_bytes(ndns, offset, buf, n, flags);
>  }
>
> @@ -48,14 +51,13 @@ static int arena_write_bytes(struct arena_info *arena, 
> resource_size_t offset,
> struct nd_btt *nd_btt = arena->nd_btt;
> struct nd_namespace_common *ndns = nd_btt->ndns;
>
> -   /* arena offsets may be shifted from the base of the device */
> -   offset += arena->nd_btt->initial_offset;
> return nvdimm_write_bytes(ndns, offset, buf, n, flags);
>  }

So let's get rid of arena_{read,write}_bytes(). The only point of the
wrapper was to include an arena specific offset, but now we're pushing
all offset calculations to the caller so arena_{read,write}_bytes() is
equivalent to nvdimm_{read,write}_bytes(). However, since many of
these call sites now perform a common offset calculations maybe we can
provide a new helper for that to differentiate it from the error
clearing case? After patch 7 is applied it's no longer clear which
offset calculation is used where and for what reason, there's just too
many offset variables calculations and overload of the word 'offset'
for bugs to hide.

Maybe it's just late on a Friday and my eyes are going cross, but it
seems you have 2 classes of use cases between calc_nsoff and
lba_to_nsoff, perhaps separate read/write wrappers for those 2 cases?
Also, I find something unreadable about calc_nsoff, and reading the
prototype "u64 calc_nsoff(struct nd_btt *nd_btt, u64 relative_offset)"
doesn't help clarify. What is 'relative_offset' relative to? Is it the
base namespace / device offset?
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm