On Thu, Aug 03, 2017 at 11:02:15AM -0500, Eric Sandeen wrote:
> On 7/31/17 4:07 PM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> >
> > Extend typ_t to (optionally) store a pointer to a function to calculate
> > the CRC of the block, provide functions to do this for the dir3 and
> > attr3 types, and then wire up the write command so that we can modify
> > directory and extended attribute block fields.
> >
> > Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> > ---
> > db/attr.c | 32 ++++++++++++++++++++++++++++++++
> > db/attr.h | 1 +
> > db/dir2.c | 37 +++++++++++++++++++++++++++++++++++++
> > db/dir2.h | 1 +
> > db/type.c | 8 ++++----
> > db/type.h | 2 ++
> > db/write.c | 3 +++
> > 7 files changed, 80 insertions(+), 4 deletions(-)
> >
> >
> > diff --git a/db/attr.c b/db/attr.c
> > index 98fb069..2fa6690 100644
> > --- a/db/attr.c
> > +++ b/db/attr.c
> > @@ -602,6 +602,38 @@ const struct field attr3_remote_crc_flds[] = {
> > { NULL }
> > };
> >
> > +/* Set the CRC. */
> > +void
> > +xfs_attr3_set_crc(
> > + struct xfs_buf *bp)
> > +{
> > + __be32 magic32;
> > + __be16 magic16;
> > +
> > + magic32 = *(__be32 *)bp->b_addr;
> > + magic16 = ((struct xfs_da_blkinfo *)bp->b_addr)->magic;
> > +
> > + switch (magic16) {
> > + case cpu_to_be16(XFS_ATTR3_LEAF_MAGIC):
>
> Isn't it more typical to endian-swap magic16 and not have the
> swap in each case statement? *shrug*
cpu_to_be*() will do constant folding so we don't have to do any byte
swapping at run time.
(I doubt it matters much here, but a fair amount of xfs code follows
that paradigm...)
> > + xfs_buf_update_cksum(bp, XFS_ATTR3_LEAF_CRC_OFF);
> > + return;
> > + case cpu_to_be16(XFS_DA3_NODE_MAGIC):
> > + xfs_buf_update_cksum(bp, XFS_DA3_NODE_CRC_OFF);
> > + return;
> > + default:
> > + break;
> > + }
>
> ...
>
> >
> > diff --git a/db/write.c b/db/write.c
> > index d24ea05..266bde4 100644
> > --- a/db/write.c
> > +++ b/db/write.c
> > @@ -173,6 +173,9 @@ write_f(
> > } else if (iocur_top->dquot_buf) {
> > local_ops.verify_write = xfs_verify_recalc_dquot_crc;
> > dbprintf(_("Allowing write of corrupted dquot with good CRC\n"));
> > + } else if (iocur_top->typ->crc_off == TYP_F_CRC_FUNC) {
> > + local_ops.verify_write = iocur_top->typ->set_crc;
> > + dbprintf(_("Allowing write of corrupted data with good CRC\n"));
> > } else { /* invalid data */
> > local_ops.verify_write = xfs_verify_recalc_crc;
> > dbprintf(_("Allowing write of corrupted data with good CRC\n"));
>
> looking at this else if else if else if gunk makes me think that this has
> grown a bit out of control.
>
> We have other special types that require unique crc setting functions - dquots
> and inodes. In those cases, we have TYP_F_NO_CRC_OFF but a special indicator
> that they are of this type (ino_buf, dquot_buf), with a hard-coded recalculation
> routine (xfs_verify_recalc_dquot_crc, xfs_verify_recalc_inode_crc). Now for /other/
> types you've extended typ_t and put the function in there as an op, which makes
> sense.
>
> It seems to me that it would be logical to move the existing crc recalculation
> routines for dquots & inodes into this new op as well, and locate & name everything
> consistently, no?
Yep. New patch!
--D
>
> Thanks,
> -Eric
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
