diff mbox series

[41/45] libxfs: always initialize internal buffer map

Message ID 164263806915.860211.11553766371419430734.stgit@magnolia (mailing list archive)
State Accepted
Headers show
Series xfsprogs: sync libxfs with 5.15 | expand

Commit Message

Darrick J. Wong Jan. 20, 2022, 12:21 a.m. UTC 
From: Darrick J. Wong <djwong@kernel.org>

The __initbuf function is responsible for initializing the fields of an
xfs_buf.  Buffers are always required to have a mapping, though in the
typical case there's only one mapping, so we can use the internal one.

The single-mapping b_maps init code at the end of the function doesn't
quite get this right though -- if a single-mapping buffer in the cache
was allowed to expire and now is being repurposed, it'll come out with
b_maps == &__b_map, in which case we incorrectly skip initializing the
map.  This has gone unnoticed until now because (AFAICT) the code paths
that use b_maps are the same ones that are called with multi-mapping
buffers, which are initialized correctly.

Anyway, the improperly initialized single-mappings will cause problems
in upcoming patches where we turn b_bn into the cache key and require
the use of b_maps[0].bm_bn for the buffer LBA.  Fix this.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 libxfs/rdwr.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Eric Sandeen Jan. 28, 2022, 8:31 p.m. UTC | #1 
On 1/19/22 6:21 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> The __initbuf function is responsible for initializing the fields of an
> xfs_buf.  Buffers are always required to have a mapping, though in the
> typical case there's only one mapping, so we can use the internal one.
> 
> The single-mapping b_maps init code at the end of the function doesn't
> quite get this right though -- if a single-mapping buffer in the cache
> was allowed to expire and now is being repurposed, it'll come out with
> b_maps == &__b_map, in which case we incorrectly skip initializing the
> map. 

In this case b_nmaps must already be 1, right. And it's the bn and
length in b_maps[0] that fail to be initialized?

I wonder, then, if it's any more clear to reorganize it just a little bit,
like:

         if (!bp->b_maps) {
                 bp->b_maps = &bp->__b_map;
                 bp->b_nmaps = 1;
         }

         if (bp->b_maps == &bp->__b_map) {
                 bp->b_maps[0].bm_bn = bp->b_bn;
                 bp->b_maps[0].bm_len = bp->b_length;
         }

because AFAICT b_nmaps only needs to be reset to 1 if we didn't already
get here with b_maps == &__b_map?

If this is just navel-gazing I can leave it as is. If you think it's
any clearer, I'll make the change. (or if I've gotten it completely wrong,
sorry!)

Thanks,
-Eric

> This has gone unnoticed until now because (AFAICT) the code paths
> that use b_maps are the same ones that are called with multi-mapping
> buffers, which are initialized correctly.
> 
> Anyway, the improperly initialized single-mappings will cause problems
> in upcoming patches where we turn b_bn into the cache key and require
> the use of b_maps[0].bm_bn for the buffer LBA.  Fix this.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>   libxfs/rdwr.c |    6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> 
> diff --git a/libxfs/rdwr.c b/libxfs/rdwr.c
> index 5086bdbc..a55e3a79 100644
> --- a/libxfs/rdwr.c
> +++ b/libxfs/rdwr.c
> @@ -251,9 +251,11 @@ __initbuf(struct xfs_buf *bp, struct xfs_buftarg *btp, xfs_daddr_t bno,
>   	bp->b_ops = NULL;
>   	INIT_LIST_HEAD(&bp->b_li_list);
>   
> -	if (!bp->b_maps) {
> -		bp->b_nmaps = 1;
> +	if (!bp->b_maps)
>   		bp->b_maps = &bp->__b_map;
> +
> +	if (bp->b_maps == &bp->__b_map) {
> +		bp->b_nmaps = 1;
>   		bp->b_maps[0].bm_bn = bp->b_bn;
>   		bp->b_maps[0].bm_len = bp->b_length;
>   	}
>
Darrick J. Wong Jan. 28, 2022, 10:03 p.m. UTC | #2 
On Fri, Jan 28, 2022 at 02:31:11PM -0600, Eric Sandeen wrote:
> On 1/19/22 6:21 PM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > The __initbuf function is responsible for initializing the fields of an
> > xfs_buf.  Buffers are always required to have a mapping, though in the
> > typical case there's only one mapping, so we can use the internal one.
> > 
> > The single-mapping b_maps init code at the end of the function doesn't
> > quite get this right though -- if a single-mapping buffer in the cache
> > was allowed to expire and now is being repurposed, it'll come out with
> > b_maps == &__b_map, in which case we incorrectly skip initializing the
> > map.
> 
> In this case b_nmaps must already be 1, right. And it's the bn and
> length in b_maps[0] that fail to be initialized?
> 
> I wonder, then, if it's any more clear to reorganize it just a little bit,
> like:
> 
>         if (!bp->b_maps) {
>                 bp->b_maps = &bp->__b_map;
>                 bp->b_nmaps = 1;
>         }
> 
>         if (bp->b_maps == &bp->__b_map) {
>                 bp->b_maps[0].bm_bn = bp->b_bn;
>                 bp->b_maps[0].bm_len = bp->b_length;
>         }
> 
> because AFAICT b_nmaps only needs to be reset to 1 if we didn't already
> get here with b_maps == &__b_map?

That would also work, though it's less obvious (to me anyway) that
b_nmaps is always 1 when bp->b_maps == &bp->__b_map.

--D

> If this is just navel-gazing I can leave it as is. If you think it's
> any clearer, I'll make the change. (or if I've gotten it completely wrong,
> sorry!)
> 
> Thanks,
> -Eric
> 
> > This has gone unnoticed until now because (AFAICT) the code paths
> > that use b_maps are the same ones that are called with multi-mapping
> > buffers, which are initialized correctly.
> > 
> > Anyway, the improperly initialized single-mappings will cause problems
> > in upcoming patches where we turn b_bn into the cache key and require
> > the use of b_maps[0].bm_bn for the buffer LBA.  Fix this.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >   libxfs/rdwr.c |    6 ++++--
> >   1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > 
> > diff --git a/libxfs/rdwr.c b/libxfs/rdwr.c
> > index 5086bdbc..a55e3a79 100644
> > --- a/libxfs/rdwr.c
> > +++ b/libxfs/rdwr.c
> > @@ -251,9 +251,11 @@ __initbuf(struct xfs_buf *bp, struct xfs_buftarg *btp, xfs_daddr_t bno,
> >   	bp->b_ops = NULL;
> >   	INIT_LIST_HEAD(&bp->b_li_list);
> > -	if (!bp->b_maps) {
> > -		bp->b_nmaps = 1;
> > +	if (!bp->b_maps)
> >   		bp->b_maps = &bp->__b_map;
> > +
> > +	if (bp->b_maps == &bp->__b_map) {
> > +		bp->b_nmaps = 1;
> >   		bp->b_maps[0].bm_bn = bp->b_bn;
> >   		bp->b_maps[0].bm_len = bp->b_length;
> >   	}
> >
Eric Sandeen Jan. 28, 2022, 10:27 p.m. UTC | #3 
On 1/28/22 4:03 PM, Darrick J. Wong wrote:
> On Fri, Jan 28, 2022 at 02:31:11PM -0600, Eric Sandeen wrote:
>> On 1/19/22 6:21 PM, Darrick J. Wong wrote:
>>> From: Darrick J. Wong <djwong@kernel.org>
>>>
>>> The __initbuf function is responsible for initializing the fields of an
>>> xfs_buf.  Buffers are always required to have a mapping, though in the
>>> typical case there's only one mapping, so we can use the internal one.
>>>
>>> The single-mapping b_maps init code at the end of the function doesn't
>>> quite get this right though -- if a single-mapping buffer in the cache
>>> was allowed to expire and now is being repurposed, it'll come out with
>>> b_maps == &__b_map, in which case we incorrectly skip initializing the
>>> map.
>>
>> In this case b_nmaps must already be 1, right. And it's the bn and
>> length in b_maps[0] that fail to be initialized?
>>
>> I wonder, then, if it's any more clear to reorganize it just a little bit,
>> like:
>>
>>          if (!bp->b_maps) {
>>                  bp->b_maps = &bp->__b_map;
>>                  bp->b_nmaps = 1;
>>          }
>>
>>          if (bp->b_maps == &bp->__b_map) {
>>                  bp->b_maps[0].bm_bn = bp->b_bn;
>>                  bp->b_maps[0].bm_len = bp->b_length;
>>          }
>>
>> because AFAICT b_nmaps only needs to be reset to 1 if we didn't already
>> get here with b_maps == &__b_map?
> 
> That would also work, though it's less obvious (to me anyway) that
> b_nmaps is always 1 when bp->b_maps == &bp->__b_map.

Ok. I'll leave it alone.

-Eric

> 
> --D
> 
>> If this is just navel-gazing I can leave it as is. If you think it's
>> any clearer, I'll make the change. (or if I've gotten it completely wrong,
>> sorry!)
>>
>> Thanks,
>> -Eric
>>
>>> This has gone unnoticed until now because (AFAICT) the code paths
>>> that use b_maps are the same ones that are called with multi-mapping
>>> buffers, which are initialized correctly.
>>>
>>> Anyway, the improperly initialized single-mappings will cause problems
>>> in upcoming patches where we turn b_bn into the cache key and require
>>> the use of b_maps[0].bm_bn for the buffer LBA.  Fix this.
>>>
>>> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
>>> ---
>>>    libxfs/rdwr.c |    6 ++++--
>>>    1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>>
>>> diff --git a/libxfs/rdwr.c b/libxfs/rdwr.c
>>> index 5086bdbc..a55e3a79 100644
>>> --- a/libxfs/rdwr.c
>>> +++ b/libxfs/rdwr.c
>>> @@ -251,9 +251,11 @@ __initbuf(struct xfs_buf *bp, struct xfs_buftarg *btp, xfs_daddr_t bno,
>>>    	bp->b_ops = NULL;
>>>    	INIT_LIST_HEAD(&bp->b_li_list);
>>> -	if (!bp->b_maps) {
>>> -		bp->b_nmaps = 1;
>>> +	if (!bp->b_maps)
>>>    		bp->b_maps = &bp->__b_map;
>>> +
>>> +	if (bp->b_maps == &bp->__b_map) {
>>> +		bp->b_nmaps = 1;
>>>    		bp->b_maps[0].bm_bn = bp->b_bn;
>>>    		bp->b_maps[0].bm_len = bp->b_length;
>>>    	}
>>>
>
Eric Sandeen Jan. 31, 2022, 8:30 p.m. UTC | #4 
On 1/19/22 6:21 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> The __initbuf function is responsible for initializing the fields of an
> xfs_buf.  Buffers are always required to have a mapping, though in the
> typical case there's only one mapping, so we can use the internal one.
> 
> The single-mapping b_maps init code at the end of the function doesn't
> quite get this right though -- if a single-mapping buffer in the cache
> was allowed to expire and now is being repurposed, it'll come out with
> b_maps == &__b_map, in which case we incorrectly skip initializing the
> map.  This has gone unnoticed until now because (AFAICT) the code paths
> that use b_maps are the same ones that are called with multi-mapping
> buffers, which are initialized correctly.
> 
> Anyway, the improperly initialized single-mappings will cause problems
> in upcoming patches where we turn b_bn into the cache key and require
> the use of b_maps[0].bm_bn for the buffer LBA.  Fix this.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>

Reviewed-by: Eric Sandeen <sandeen@redhat.com>
diff mbox series

Patch

diff --git a/libxfs/rdwr.c b/libxfs/rdwr.c
index 5086bdbc..a55e3a79 100644
--- a/libxfs/rdwr.c
+++ b/libxfs/rdwr.c
@@ -251,9 +251,11 @@  __initbuf(struct xfs_buf *bp, struct xfs_buftarg *btp, xfs_daddr_t bno,
 	bp->b_ops = NULL;
 	INIT_LIST_HEAD(&bp->b_li_list);
 
-	if (!bp->b_maps) {
-		bp->b_nmaps = 1;
+	if (!bp->b_maps)
 		bp->b_maps = &bp->__b_map;
+
+	if (bp->b_maps == &bp->__b_map) {
+		bp->b_nmaps = 1;
 		bp->b_maps[0].bm_bn = bp->b_bn;
 		bp->b_maps[0].bm_len = bp->b_length;
 	}