[PATCH] xfs_db: take BB cluster offset into account when using 'type' cmd

[PATCH] xfs_db: take BB cluster offset into account when using 'type' cmd

[Andrey Albershteyn]

Changing the interpretation type of data under the cursor moves the
cursor to the beginning of BB cluster. When cursor is set to an
inode the cursor is offset in BB buffer. However, this offset is not
considered when type of the data is changed - the cursor points to
the beginning of BB buffer. For example:

$ xfs_db -c "inode 131" -c "daddr" -c "type text" \
	-c "daddr" /dev/sdb1
current daddr is 131
current daddr is 128

Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com>
---
 db/io.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/db/io.c b/db/io.c
index df97ed91..107f2e11 100644
--- a/db/io.c
+++ b/db/io.c
@@ -589,6 +589,7 @@ set_iocur_type(
 	const typ_t	*type)
 {
 	int		bb_count = 1;	/* type's size in basic blocks */
+	int boff = iocur_top->boff;
 
 	/*
 	 * Inodes are special; verifier checks all inodes in the chunk, the
@@ -613,6 +614,9 @@ set_iocur_type(
 		bb_count = BTOBB(byteize(fsize(type->fields,
 				       iocur_top->data, 0, 0)));
 	set_cur(type, iocur_top->bb, bb_count, DB_RING_IGN, NULL);
+	iocur_top->boff = boff;
+	iocur_top->off = ((xfs_off_t)iocur_top->bb << BBSHIFT) + boff;
+	iocur_top->data = (void *)((char *)iocur_top->buf + boff);
 }
 
 static void
-- 
2.27.0

Re: [PATCH] xfs_db: take BB cluster offset into account when using 'type' cmd

[Darrick J. Wong]

On Tue, Apr 19, 2022 at 02:19:51PM +0200, Andrey Albershteyn wrote:
> Changing the interpretation type of data under the cursor moves the
> cursor to the beginning of BB cluster. When cursor is set to an
> inode the cursor is offset in BB buffer. However, this offset is not
> considered when type of the data is changed - the cursor points to
> the beginning of BB buffer. For example:
> 
> $ xfs_db -c "inode 131" -c "daddr" -c "type text" \
> 	-c "daddr" /dev/sdb1
> current daddr is 131
> current daddr is 128
> 
> Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com>
> ---
>  db/io.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/db/io.c b/db/io.c
> index df97ed91..107f2e11 100644
> --- a/db/io.c
> +++ b/db/io.c
> @@ -589,6 +589,7 @@ set_iocur_type(
>  	const typ_t	*type)
>  {
>  	int		bb_count = 1;	/* type's size in basic blocks */
> +	int boff = iocur_top->boff;

Nit: Please line up the variable names.

>  
>  	/*
>  	 * Inodes are special; verifier checks all inodes in the chunk, the
> @@ -613,6 +614,9 @@ set_iocur_type(
>  		bb_count = BTOBB(byteize(fsize(type->fields,
>  				       iocur_top->data, 0, 0)));
>  	set_cur(type, iocur_top->bb, bb_count, DB_RING_IGN, NULL);
> +	iocur_top->boff = boff;
> +	iocur_top->off = ((xfs_off_t)iocur_top->bb << BBSHIFT) + boff;
> +	iocur_top->data = (void *)((char *)iocur_top->buf + boff);

It seems reasonable to me to preserve the io cursor's boff when we're
setting /only/ the buffer type, but this function and off_cur() could
share these three lines of code that (re)set boff/off/data.

Alternately, I guess you could just call off_cur(boff, BBTOB(bb_count))
here.

--D

>  }
>  
>  static void
> -- 
> 2.27.0
> 

Re: [PATCH] xfs_db: take BB cluster offset into account when using 'type' cmd

[Andrey Albershteyn]

On Tue, Apr 19, 2022 at 08:47:50AM -0700, Darrick J. Wong wrote:
> On Tue, Apr 19, 2022 at 02:19:51PM +0200, Andrey Albershteyn wrote:
> > Changing the interpretation type of data under the cursor moves the
> > cursor to the beginning of BB cluster. When cursor is set to an
> > inode the cursor is offset in BB buffer. However, this offset is not
> > considered when type of the data is changed - the cursor points to
> > the beginning of BB buffer. For example:
> > 
> > $ xfs_db -c "inode 131" -c "daddr" -c "type text" \
> > 	-c "daddr" /dev/sdb1
> > current daddr is 131
> > current daddr is 128
> > 
> > Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com>
> > ---
> >  db/io.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/db/io.c b/db/io.c
> > index df97ed91..107f2e11 100644
> > --- a/db/io.c
> > +++ b/db/io.c
> > @@ -589,6 +589,7 @@ set_iocur_type(
> >  	const typ_t	*type)
> >  {
> >  	int		bb_count = 1;	/* type's size in basic blocks */
> > +	int boff = iocur_top->boff;
> 
> Nit: Please line up the variable names.

sure ;)

> 
> >  
> >  	/*
> >  	 * Inodes are special; verifier checks all inodes in the chunk, the
> > @@ -613,6 +614,9 @@ set_iocur_type(
> >  		bb_count = BTOBB(byteize(fsize(type->fields,
> >  				       iocur_top->data, 0, 0)));
> >  	set_cur(type, iocur_top->bb, bb_count, DB_RING_IGN, NULL);
> > +	iocur_top->boff = boff;
> > +	iocur_top->off = ((xfs_off_t)iocur_top->bb << BBSHIFT) + boff;
> > +	iocur_top->data = (void *)((char *)iocur_top->buf + boff);
> 
> It seems reasonable to me to preserve the io cursor's boff when we're
> setting /only/ the buffer type, but this function and off_cur() could
> share these three lines of code that (re)set boff/off/data.
> 
> Alternately, I guess you could just call off_cur(boff, BBTOB(bb_count))
> here.

This won't pass the second condition in off_cur(). I suppose the
purpose of off_cur() was to shift io cursor in BB buffer. But
changing the type changes the blen which could be smaller (e.g.
inode blen == 32 -> text blen == 1). Anyway, will try to come up
with a meaningful name for this 3 lines function :)

I think the other solution could be to set boff in set_cur(), but
this will require more refactoring and I suppose this is the only
place where newly added argument would be used.

> 
> --D
> 
> >  }
> >  
> >  static void
> > -- 
> > 2.27.0
> > 
> 

-- 
- Andrey

Re: [PATCH] xfs_db: take BB cluster offset into account when using 'type' cmd

[Darrick J. Wong]

On Tue, Apr 19, 2022 at 06:56:00PM +0200, Andrey Albershteyn wrote:
> On Tue, Apr 19, 2022 at 08:47:50AM -0700, Darrick J. Wong wrote:
> > On Tue, Apr 19, 2022 at 02:19:51PM +0200, Andrey Albershteyn wrote:
> > > Changing the interpretation type of data under the cursor moves the
> > > cursor to the beginning of BB cluster. When cursor is set to an
> > > inode the cursor is offset in BB buffer. However, this offset is not
> > > considered when type of the data is changed - the cursor points to
> > > the beginning of BB buffer. For example:
> > > 
> > > $ xfs_db -c "inode 131" -c "daddr" -c "type text" \
> > > 	-c "daddr" /dev/sdb1
> > > current daddr is 131
> > > current daddr is 128
> > > 
> > > Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com>
> > > ---
> > >  db/io.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/db/io.c b/db/io.c
> > > index df97ed91..107f2e11 100644
> > > --- a/db/io.c
> > > +++ b/db/io.c
> > > @@ -589,6 +589,7 @@ set_iocur_type(
> > >  	const typ_t	*type)
> > >  {
> > >  	int		bb_count = 1;	/* type's size in basic blocks */
> > > +	int boff = iocur_top->boff;
> > 
> > Nit: Please line up the variable names.
> 
> sure ;)
> 
> > 
> > >  
> > >  	/*
> > >  	 * Inodes are special; verifier checks all inodes in the chunk, the
> > > @@ -613,6 +614,9 @@ set_iocur_type(
> > >  		bb_count = BTOBB(byteize(fsize(type->fields,
> > >  				       iocur_top->data, 0, 0)));
> > >  	set_cur(type, iocur_top->bb, bb_count, DB_RING_IGN, NULL);
> > > +	iocur_top->boff = boff;
> > > +	iocur_top->off = ((xfs_off_t)iocur_top->bb << BBSHIFT) + boff;
> > > +	iocur_top->data = (void *)((char *)iocur_top->buf + boff);
> > 
> > It seems reasonable to me to preserve the io cursor's boff when we're
> > setting /only/ the buffer type, but this function and off_cur() could
> > share these three lines of code that (re)set boff/off/data.
> > 
> > Alternately, I guess you could just call off_cur(boff, BBTOB(bb_count))
> > here.
> 
> This won't pass the second condition in off_cur(). I suppose the
> purpose of off_cur() was to shift io cursor in BB buffer. But
> changing the type changes the blen which could be smaller (e.g.
> inode blen == 32 -> text blen == 1). Anyway, will try to come up
> with a meaningful name for this 3 lines function :)

Ooh, a bikeshed!  How about:

static inline void set_cur_boff(int boff)
{
	iocur_top->boff = boff;
	iocur_top->off = ((xfs_off_t)iocur_top->bb << BBSHIFT) + boff;
	iocur_top->data = (void *)((char *)iocur_top->buf + boff);
}

> I think the other solution could be to set boff in set_cur(), but
> this will require more refactoring and I suppose this is the only
> place where newly added argument would be used.
> 
> > 
> > --D
> > 
> > >  }
> > >  
> > >  static void
> > > -- 
> > > 2.27.0
> > > 
> > 
> 
> -- 
> - Andrey
>