| Message ID | 20170622193352.22346-1-billodo@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Thu, Jun 22, 2017 at 02:33:52PM -0500, Bill O'Donnell wrote: > xfs_db doesn't take sector size into account when setting type. > This can result in an errant crc. For example, with a sector size > of 4096: > > xfs_db> agi 0 > xfs_db> p crc > crc = 0xab85043e (correct) > xfs_db> daddr > current daddr is 16 > xfs_db> daddr 42 > xfs_db> daddr 16 > xfs_db> type agi > Metadata CRC error detected at xfs_agi block 0x10/0x200 > xfs_db> p crc > crc = 0xab85043e (bad) > > When xfs_db sets the new daddr in daddr_f, it does so with one > BBSIZE sector (512). Changing the type doesn't change the size > of the current buffer in iocur_top, so the checksum is calculated > on the wrong length for the type (when the actual sector size > BBSIZE (512). > > For types with fields, reread the buffer to pick up the correct size for > the new type when it gets set. Facilitate the reread by setting the cursor > with set_cur(). > > Signed-off-by: Bill O'Donnell <billodo@redhat.com> > --- > db/io.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/db/io.c b/db/io.c > index 9918a51..7e6d330 100644 > --- a/db/io.c > +++ b/db/io.c > @@ -616,6 +616,12 @@ set_iocur_type( > { > struct xfs_buf *bp = iocur_top->bp; > > + if (t->fields) > + set_cur(t, > + iocur_top->bb, > + fsize(t->fields, iocur_top->data, > + 0, 0) / mp->m_sb.sb_blocksize, I thought the third parameter to set_cur (which is fed as the third parameter to libxfs_readbuf) was expressed in units of basic blocks (i.e. 512 bytes)? fsize returns the size of the field in bytes (I think?), so dividing by sb_blocksize renders units of fs blocks, not basic blocks. <shrug> the naming isn't helpful at all: void set_cur( const typ_t *t, int64_t d, int c, ... iocur_top->len = BBTOB(c); <codeconfused> --D > + DB_RING_IGN, NULL); > iocur_top->typ = t; > > /* verify the buffer if the type has one. */ > -- > 2.9.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 6/22/17 2:47 PM, Darrick J. Wong wrote: > On Thu, Jun 22, 2017 at 02:33:52PM -0500, Bill O'Donnell wrote: >> xfs_db doesn't take sector size into account when setting type. >> This can result in an errant crc. For example, with a sector size >> of 4096: >> >> xfs_db> agi 0 >> xfs_db> p crc >> crc = 0xab85043e (correct) >> xfs_db> daddr >> current daddr is 16 >> xfs_db> daddr 42 >> xfs_db> daddr 16 >> xfs_db> type agi >> Metadata CRC error detected at xfs_agi block 0x10/0x200 >> xfs_db> p crc >> crc = 0xab85043e (bad) >> >> When xfs_db sets the new daddr in daddr_f, it does so with one >> BBSIZE sector (512). Changing the type doesn't change the size >> of the current buffer in iocur_top, so the checksum is calculated >> on the wrong length for the type (when the actual sector size > BBSIZE (512). >> >> For types with fields, reread the buffer to pick up the correct size for >> the new type when it gets set. Facilitate the reread by setting the cursor >> with set_cur(). >> >> Signed-off-by: Bill O'Donnell <billodo@redhat.com> >> --- >> db/io.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/db/io.c b/db/io.c >> index 9918a51..7e6d330 100644 >> --- a/db/io.c >> +++ b/db/io.c >> @@ -616,6 +616,12 @@ set_iocur_type( >> { >> struct xfs_buf *bp = iocur_top->bp; >> a comment about why only this "if" case is there would be good. >> + if (t->fields) >> + set_cur(t, >> + iocur_top->bb, >> + fsize(t->fields, iocur_top->data, >> + 0, 0) / mp->m_sb.sb_blocksize, > > I thought the third parameter to set_cur (which is fed as the third > parameter to libxfs_readbuf) was expressed in units of basic blocks > (i.e. 512 bytes)? fsize returns the size of the field in bytes (I > think?), nope, bits :) > so dividing by sb_blocksize renders units of fs blocks, not > basic blocks. As Darrick just pointed out to me, you treated bits as bytes and daddr as fsblock so for 4k blocks, 8/8 came out right ;) I think what you want for the size is something like: BTOBB(byteize(fsize(t->fields, iocur_top->data, 0, 0))); It might be a little nicer to stash that in a temp var so you don't have all that gunk in the call to set_cur. > <shrug> the naming isn't helpful at all: > > void > set_cur( > const typ_t *t, > int64_t d, > int c, > ... > iocur_top->len = BBTOB(c); > > <codeconfused> yup isn't xfs_db fun? > > --D > >> + DB_RING_IGN, NULL); >> iocur_top->typ = t; >> >> /* verify the buffer if the type has one. */ >> -- >> 2.9.4 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 22, 2017 at 12:47:23PM -0700, Darrick J. Wong wrote: > On Thu, Jun 22, 2017 at 02:33:52PM -0500, Bill O'Donnell wrote: > > xfs_db doesn't take sector size into account when setting type. > > This can result in an errant crc. For example, with a sector size > > of 4096: > > > > xfs_db> agi 0 > > xfs_db> p crc > > crc = 0xab85043e (correct) > > xfs_db> daddr > > current daddr is 16 > > xfs_db> daddr 42 > > xfs_db> daddr 16 > > xfs_db> type agi > > Metadata CRC error detected at xfs_agi block 0x10/0x200 > > xfs_db> p crc > > crc = 0xab85043e (bad) > > > > When xfs_db sets the new daddr in daddr_f, it does so with one > > BBSIZE sector (512). Changing the type doesn't change the size > > of the current buffer in iocur_top, so the checksum is calculated > > on the wrong length for the type (when the actual sector size > BBSIZE (512). > > > > For types with fields, reread the buffer to pick up the correct size for > > the new type when it gets set. Facilitate the reread by setting the cursor > > with set_cur(). > > > > Signed-off-by: Bill O'Donnell <billodo@redhat.com> > > --- > > db/io.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/db/io.c b/db/io.c > > index 9918a51..7e6d330 100644 > > --- a/db/io.c > > +++ b/db/io.c > > @@ -616,6 +616,12 @@ set_iocur_type( > > { > > struct xfs_buf *bp = iocur_top->bp; > > > > + if (t->fields) > > + set_cur(t, > > + iocur_top->bb, > > + fsize(t->fields, iocur_top->data, > > + 0, 0) / mp->m_sb.sb_blocksize, > > I thought the third parameter to set_cur (which is fed as the third > parameter to libxfs_readbuf) was expressed in units of basic blocks > (i.e. 512 bytes)? fsize returns the size of the field in bytes (I > think?), so dividing by sb_blocksize renders units of fs blocks, not > basic blocks. > fsize returns the field size in bits. Dividing by sb_blocksize gives basic blocks. > <shrug> the naming isn't helpful at all: I also found the naming through the call stack to be terrible and damned confusing (c, d ? really??), but left that battle for another day, another patch. ;) Thanks- Bill > > void > set_cur( > const typ_t *t, > int64_t d, > int c, > ... > iocur_top->len = BBTOB(c); > > <codeconfused> > > --D > > > + DB_RING_IGN, NULL); > > iocur_top->typ = t; > > > > /* verify the buffer if the type has one. */ > > -- > > 2.9.4 > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 22, 2017 at 03:16:27PM -0500, Eric Sandeen wrote: > > > On 6/22/17 2:47 PM, Darrick J. Wong wrote: > > On Thu, Jun 22, 2017 at 02:33:52PM -0500, Bill O'Donnell wrote: > >> xfs_db doesn't take sector size into account when setting type. > >> This can result in an errant crc. For example, with a sector size > >> of 4096: > >> > >> xfs_db> agi 0 > >> xfs_db> p crc > >> crc = 0xab85043e (correct) > >> xfs_db> daddr > >> current daddr is 16 > >> xfs_db> daddr 42 > >> xfs_db> daddr 16 > >> xfs_db> type agi > >> Metadata CRC error detected at xfs_agi block 0x10/0x200 > >> xfs_db> p crc > >> crc = 0xab85043e (bad) > >> > >> When xfs_db sets the new daddr in daddr_f, it does so with one > >> BBSIZE sector (512). Changing the type doesn't change the size > >> of the current buffer in iocur_top, so the checksum is calculated > >> on the wrong length for the type (when the actual sector size > BBSIZE (512). > >> > >> For types with fields, reread the buffer to pick up the correct size for > >> the new type when it gets set. Facilitate the reread by setting the cursor > >> with set_cur(). > >> > >> Signed-off-by: Bill O'Donnell <billodo@redhat.com> > >> --- > >> db/io.c | 6 ++++++ > >> 1 file changed, 6 insertions(+) > >> > >> diff --git a/db/io.c b/db/io.c > >> index 9918a51..7e6d330 100644 > >> --- a/db/io.c > >> +++ b/db/io.c > >> @@ -616,6 +616,12 @@ set_iocur_type( > >> { > >> struct xfs_buf *bp = iocur_top->bp; > >> > > a comment about why only this "if" case is there would be good. agreed. > > >> + if (t->fields) > >> + set_cur(t, > >> + iocur_top->bb, > >> + fsize(t->fields, iocur_top->data, > >> + 0, 0) / mp->m_sb.sb_blocksize, > > > > I thought the third parameter to set_cur (which is fed as the third > > parameter to libxfs_readbuf) was expressed in units of basic blocks > > (i.e. 512 bytes)? fsize returns the size of the field in bytes (I > > think?), > > nope, bits :) > > > so dividing by sb_blocksize renders units of fs blocks, not > > basic blocks. > > As Darrick just pointed out to me, you treated bits as bytes and > daddr as fsblock so for 4k blocks, 8/8 came out right ;) Yeah. Doh! I tested with sector sizes 512 and 4096 and it all just worked, since mp->m_sb.sb_blocksize returned 1 and 8, respectively. > > I think what you want for the size is something like: > > BTOBB(byteize(fsize(t->fields, iocur_top->data, 0, 0))); > > It might be a little nicer to stash that in a temp var so you > don't have all that gunk in the call to set_cur. Yep. > > <shrug> the naming isn't helpful at all: > > > > void > > set_cur( > > const typ_t *t, > > int64_t d, > > int c, > > ... > > iocur_top->len = BBTOB(c); > > > > <codeconfused> > > yup isn't xfs_db fun? > > > > > --D > > > >> + DB_RING_IGN, NULL); > >> iocur_top->typ = t; > >> > >> /* verify the buffer if the type has one. */ > >> -- > >> 2.9.4 > >> > >> -- > >> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > >> the body of a message to majordomo@vger.kernel.org > >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 22, 2017 at 03:31:08PM -0500, Bill O'Donnell wrote: > On Thu, Jun 22, 2017 at 03:16:27PM -0500, Eric Sandeen wrote: > > > > > > On 6/22/17 2:47 PM, Darrick J. Wong wrote: > > > On Thu, Jun 22, 2017 at 02:33:52PM -0500, Bill O'Donnell wrote: > > >> xfs_db doesn't take sector size into account when setting type. > > >> This can result in an errant crc. For example, with a sector size > > >> of 4096: > > >> > > >> xfs_db> agi 0 > > >> xfs_db> p crc > > >> crc = 0xab85043e (correct) > > >> xfs_db> daddr > > >> current daddr is 16 > > >> xfs_db> daddr 42 > > >> xfs_db> daddr 16 > > >> xfs_db> type agi > > >> Metadata CRC error detected at xfs_agi block 0x10/0x200 > > >> xfs_db> p crc > > >> crc = 0xab85043e (bad) > > >> > > >> When xfs_db sets the new daddr in daddr_f, it does so with one > > >> BBSIZE sector (512). Changing the type doesn't change the size > > >> of the current buffer in iocur_top, so the checksum is calculated > > >> on the wrong length for the type (when the actual sector size > BBSIZE (512). > > >> > > >> For types with fields, reread the buffer to pick up the correct size for > > >> the new type when it gets set. Facilitate the reread by setting the cursor > > >> with set_cur(). > > >> > > >> Signed-off-by: Bill O'Donnell <billodo@redhat.com> > > >> --- > > >> db/io.c | 6 ++++++ > > >> 1 file changed, 6 insertions(+) > > >> > > >> diff --git a/db/io.c b/db/io.c > > >> index 9918a51..7e6d330 100644 > > >> --- a/db/io.c > > >> +++ b/db/io.c > > >> @@ -616,6 +616,12 @@ set_iocur_type( > > >> { > > >> struct xfs_buf *bp = iocur_top->bp; > > >> > > > > a comment about why only this "if" case is there would be good. > > agreed. > > > > > >> + if (t->fields) > > >> + set_cur(t, > > >> + iocur_top->bb, > > >> + fsize(t->fields, iocur_top->data, > > >> + 0, 0) / mp->m_sb.sb_blocksize, > > > > > > I thought the third parameter to set_cur (which is fed as the third > > > parameter to libxfs_readbuf) was expressed in units of basic blocks > > > (i.e. 512 bytes)? fsize returns the size of the field in bytes (I > > > think?), > > > > nope, bits :) > > > > > so dividing by sb_blocksize renders units of fs blocks, not > > > basic blocks. > > > > As Darrick just pointed out to me, you treated bits as bytes and > > daddr as fsblock so for 4k blocks, 8/8 came out right ;) > > Yeah. Doh! I tested with sector sizes 512 and 4096 and it all just worked, > since mp->m_sb.sb_blocksize returned 1 and 8, respectively. ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Correction: _dividing by mp->m_sb.sb_blocksize_ gave 1 and 8, respectively. > > > > > I think what you want for the size is something like: > > > > BTOBB(byteize(fsize(t->fields, iocur_top->data, 0, 0))); > > > > It might be a little nicer to stash that in a temp var so you > > don't have all that gunk in the call to set_cur. > > Yep. > > > > > <shrug> the naming isn't helpful at all: > > > > > > void > > > set_cur( > > > const typ_t *t, > > > int64_t d, > > > int c, > > > ... > > > iocur_top->len = BBTOB(c); > > > > > > <codeconfused> > > > > yup isn't xfs_db fun? > > > > > > > > --D > > > > > >> + DB_RING_IGN, NULL); > > >> iocur_top->typ = t; > > >> > > >> /* verify the buffer if the type has one. */ > > >> -- > > >> 2.9.4 > > >> > > >> -- > > >> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > > >> the body of a message to majordomo@vger.kernel.org > > >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > > -- > > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > > > the body of a message to majordomo@vger.kernel.org > > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 22, 2017 at 03:23:22PM -0500, Bill O'Donnell wrote: > On Thu, Jun 22, 2017 at 12:47:23PM -0700, Darrick J. Wong wrote: > > On Thu, Jun 22, 2017 at 02:33:52PM -0500, Bill O'Donnell wrote: > > > xfs_db doesn't take sector size into account when setting type. > > > This can result in an errant crc. For example, with a sector size > > > of 4096: > > > > > > xfs_db> agi 0 > > > xfs_db> p crc > > > crc = 0xab85043e (correct) > > > xfs_db> daddr > > > current daddr is 16 > > > xfs_db> daddr 42 > > > xfs_db> daddr 16 > > > xfs_db> type agi > > > Metadata CRC error detected at xfs_agi block 0x10/0x200 > > > xfs_db> p crc > > > crc = 0xab85043e (bad) > > > > > > When xfs_db sets the new daddr in daddr_f, it does so with one > > > BBSIZE sector (512). Changing the type doesn't change the size > > > of the current buffer in iocur_top, so the checksum is calculated > > > on the wrong length for the type (when the actual sector size > BBSIZE (512). > > > > > > For types with fields, reread the buffer to pick up the correct size for > > > the new type when it gets set. Facilitate the reread by setting the cursor > > > with set_cur(). > > > > > > Signed-off-by: Bill O'Donnell <billodo@redhat.com> > > > --- > > > db/io.c | 6 ++++++ > > > 1 file changed, 6 insertions(+) > > > > > > diff --git a/db/io.c b/db/io.c > > > index 9918a51..7e6d330 100644 > > > --- a/db/io.c > > > +++ b/db/io.c > > > @@ -616,6 +616,12 @@ set_iocur_type( > > > { > > > struct xfs_buf *bp = iocur_top->bp; > > > > > > + if (t->fields) > > > + set_cur(t, > > > + iocur_top->bb, > > > + fsize(t->fields, iocur_top->data, > > > + 0, 0) / mp->m_sb.sb_blocksize, > > > > I thought the third parameter to set_cur (which is fed as the third > > parameter to libxfs_readbuf) was expressed in units of basic blocks > > (i.e. 512 bytes)? fsize returns the size of the field in bytes (I > > think?), so dividing by sb_blocksize renders units of fs blocks, not > > basic blocks. > > > > fsize returns the field size in bits. Dividing by sb_blocksize gives basic blocks. Yes, the subtlety that fsize() returns bits and sb_blocksize is usually 4096 / 512 == 8 means that the conversion works for the default case, but that leaves us open to bugs. Consider if sb_blocksize == 65536. There are so many different unit types in XFS that it is crucial for auditing to make analyzing unit conversions as easy as possible. That means that we must show all intermediate steps in converting from one unit system to another, and we must not short circuit those hops. This helps the reviewers find bugs. For example, consider Eric's suggested replacement for the third parameter: BTOBB(byteize(fsize(t->fields, iocur_top->data, 0, 0))); We know that the units of the third parameter are in basic blocks from flailing around in the set_cur code due to unhelpful parameter names. BTOBB converts bytes to basic blocks, so we theorize that the input must be in units of bytes. We can then unpack the next level: byteize(fsize(...)) Starting from the premise that this expression is in units of bytes, we apply the knowledge that byteize converts bit counts to byte counts, so we now also theorize that the input must be in units of bits. Unpack the next level: fsize(...) To finish checking the expression value, we simply have to confirm that fsize() returns units of bits. Therefore, the expression is correct. The unit analysis is straightforward and does not require knowledge of subtleties. --D > > > <shrug> the naming isn't helpful at all: > > I also found the naming through the call stack to be terrible and damned > confusing (c, d ? really??), but left that battle for another day, another > patch. ;) > > Thanks- > Bill > > > > > void > > set_cur( > > const typ_t *t, > > int64_t d, > > int c, > > ... > > iocur_top->len = BBTOB(c); > > > > <codeconfused> > > > > --D > > > > > + DB_RING_IGN, NULL); > > > iocur_top->typ = t; > > > > > > /* verify the buffer if the type has one. */ > > > -- > > > 2.9.4 > > > > > > -- > > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > > > the body of a message to majordomo@vger.kernel.org > > > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 22, 2017 at 01:39:41PM -0700, Darrick J. Wong wrote: > On Thu, Jun 22, 2017 at 03:23:22PM -0500, Bill O'Donnell wrote: > > On Thu, Jun 22, 2017 at 12:47:23PM -0700, Darrick J. Wong wrote: > > > On Thu, Jun 22, 2017 at 02:33:52PM -0500, Bill O'Donnell wrote: > > > > xfs_db doesn't take sector size into account when setting type. > > > > This can result in an errant crc. For example, with a sector size > > > > of 4096: > > > > > > > > xfs_db> agi 0 > > > > xfs_db> p crc > > > > crc = 0xab85043e (correct) > > > > xfs_db> daddr > > > > current daddr is 16 > > > > xfs_db> daddr 42 > > > > xfs_db> daddr 16 > > > > xfs_db> type agi > > > > Metadata CRC error detected at xfs_agi block 0x10/0x200 > > > > xfs_db> p crc > > > > crc = 0xab85043e (bad) > > > > > > > > When xfs_db sets the new daddr in daddr_f, it does so with one > > > > BBSIZE sector (512). Changing the type doesn't change the size > > > > of the current buffer in iocur_top, so the checksum is calculated > > > > on the wrong length for the type (when the actual sector size > BBSIZE (512). > > > > > > > > For types with fields, reread the buffer to pick up the correct size for > > > > the new type when it gets set. Facilitate the reread by setting the cursor > > > > with set_cur(). > > > > > > > > Signed-off-by: Bill O'Donnell <billodo@redhat.com> > > > > --- > > > > db/io.c | 6 ++++++ > > > > 1 file changed, 6 insertions(+) > > > > > > > > diff --git a/db/io.c b/db/io.c > > > > index 9918a51..7e6d330 100644 > > > > --- a/db/io.c > > > > +++ b/db/io.c > > > > @@ -616,6 +616,12 @@ set_iocur_type( > > > > { > > > > struct xfs_buf *bp = iocur_top->bp; > > > > > > > > + if (t->fields) > > > > + set_cur(t, > > > > + iocur_top->bb, > > > > + fsize(t->fields, iocur_top->data, > > > > + 0, 0) / mp->m_sb.sb_blocksize, > > > > > > I thought the third parameter to set_cur (which is fed as the third > > > parameter to libxfs_readbuf) was expressed in units of basic blocks > > > (i.e. 512 bytes)? fsize returns the size of the field in bytes (I > > > think?), so dividing by sb_blocksize renders units of fs blocks, not > > > basic blocks. > > > > > > > fsize returns the field size in bits. Dividing by sb_blocksize gives basic blocks. > > Yes, the subtlety that fsize() returns bits and sb_blocksize is usually > 4096 / 512 == 8 means that the conversion works for the default case, > but that leaves us open to bugs. Consider if sb_blocksize == 65536. > > There are so many different unit types in XFS that it is crucial for > auditing to make analyzing unit conversions as easy as possible. That > means that we must show all intermediate steps in converting from one > unit system to another, and we must not short circuit those hops. This > helps the reviewers find bugs. > > For example, consider Eric's suggested replacement for the third parameter: > > BTOBB(byteize(fsize(t->fields, iocur_top->data, 0, 0))); > > We know that the units of the third parameter are in basic blocks from > flailing around in the set_cur code due to unhelpful parameter names. > BTOBB converts bytes to basic blocks, so we theorize that the input must > be in units of bytes. We can then unpack the next level: > > byteize(fsize(...)) > > Starting from the premise that this expression is in units of bytes, we > apply the knowledge that byteize converts bit counts to byte counts, so > we now also theorize that the input must be in units of bits. Unpack > the next level: > > fsize(...) > > To finish checking the expression value, we simply have to confirm that > fsize() returns units of bits. Therefore, the expression is correct. > The unit analysis is straightforward and does not require knowledge of > subtleties. Excellent analysis, thanks! Eric suggested I come up with a (robust) test case, and as you point out, the need is fairly obvious. Thanks- Bill > > --D > > > > > > <shrug> the naming isn't helpful at all: > > > > I also found the naming through the call stack to be terrible and damned > > confusing (c, d ? really??), but left that battle for another day, another > > patch. ;) > > > > Thanks- > > Bill > > > > > > > > void > > > set_cur( > > > const typ_t *t, > > > int64_t d, > > > int c, > > > ... > > > iocur_top->len = BBTOB(c); > > > > > > <codeconfused> > > > > > > --D > > > > > > > + DB_RING_IGN, NULL); > > > > iocur_top->typ = t; > > > > > > > > /* verify the buffer if the type has one. */ > > > > -- > > > > 2.9.4 > > > > > > > > -- > > > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > > > > the body of a message to majordomo@vger.kernel.org > > > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/db/io.c b/db/io.c index 9918a51..7e6d330 100644 --- a/db/io.c +++ b/db/io.c @@ -616,6 +616,12 @@ set_iocur_type( { struct xfs_buf *bp = iocur_top->bp; + if (t->fields) + set_cur(t, + iocur_top->bb, + fsize(t->fields, iocur_top->data, + 0, 0) / mp->m_sb.sb_blocksize, + DB_RING_IGN, NULL); iocur_top->typ = t; /* verify the buffer if the type has one. */
xfs_db doesn't take sector size into account when setting type. This can result in an errant crc. For example, with a sector size of 4096: xfs_db> agi 0 xfs_db> p crc crc = 0xab85043e (correct) xfs_db> daddr current daddr is 16 xfs_db> daddr 42 xfs_db> daddr 16 xfs_db> type agi Metadata CRC error detected at xfs_agi block 0x10/0x200 xfs_db> p crc crc = 0xab85043e (bad) When xfs_db sets the new daddr in daddr_f, it does so with one BBSIZE sector (512). Changing the type doesn't change the size of the current buffer in iocur_top, so the checksum is calculated on the wrong length for the type (when the actual sector size > BBSIZE (512). For types with fields, reread the buffer to pick up the correct size for the new type when it gets set. Facilitate the reread by setting the cursor with set_cur(). Signed-off-by: Bill O'Donnell <billodo@redhat.com> --- db/io.c | 6 ++++++ 1 file changed, 6 insertions(+)