| Message ID | 20200417150859.14734-7-bfoster@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xfs: flush related error handling cleanups | expand |
On Fri, Apr 17, 2020 at 11:08:53AM -0400, Brian Foster wrote: > The dquot read/write verifier calls xfs_dqblk_verify() on every > dquot in the buffer. Remove the duplicate call from > xfs_qm_dqflush(). Ah, I think there's a bug here - it's not supposed to be a duplicate.... > Signed-off-by: Brian Foster <bfoster@redhat.com> > --- > fs/xfs/xfs_dquot.c | 14 -------------- > 1 file changed, 14 deletions(-) > > diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c > index af2c8e5ceea0..73032c18a94a 100644 > --- a/fs/xfs/xfs_dquot.c > +++ b/fs/xfs/xfs_dquot.c > @@ -1071,7 +1071,6 @@ xfs_qm_dqflush( > struct xfs_buf *bp; > struct xfs_dqblk *dqb; > struct xfs_disk_dquot *ddqp; > - xfs_failaddr_t fa; > int error; > > ASSERT(XFS_DQ_IS_LOCKED(dqp)); > @@ -1116,19 +1115,6 @@ xfs_qm_dqflush( > dqb = bp->b_addr + dqp->q_bufoffset; > ddqp = &dqb->dd_diskdq; > > - /* > - * A simple sanity check in case we got a corrupted dquot. > - */ > - fa = xfs_dqblk_verify(mp, dqb, be32_to_cpu(ddqp->d_id), 0); So this verifies the on disk dquot .... > - if (fa) { > - xfs_alert(mp, "corrupt dquot ID 0x%x in memory at %pS", ...which issues an "in memory corruption" alert on failure... > - be32_to_cpu(ddqp->d_id), fa); > - xfs_buf_relse(bp); > - xfs_dqfunlock(dqp); > - xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE); > - return -EFSCORRUPTED; > - } > - > /* This is the only portion of data that needs to persist */ > memcpy(ddqp, &dqp->q_core, sizeof(struct xfs_disk_dquot)); .... and on success we immediately overwrite the on-disk copy with the unchecked in-memory copy of the dquot. IOWs, I think that verification call here should be checking the in-memory dquot core, not the on disk buffer that is about to get trashed. i.e. something like this: - fa = xfs_dqblk_verify(mp, dqb, be32_to_cpu(ddqp->d_id), 0); + fa = xfs_dquot_verify(mp, &dqp->q_core, be32_to_cpu(ddqp->d_id), 0); Cheers, Dave.
On Mon, Apr 20, 2020 at 01:53:22PM +1000, Dave Chinner wrote: > On Fri, Apr 17, 2020 at 11:08:53AM -0400, Brian Foster wrote: > > The dquot read/write verifier calls xfs_dqblk_verify() on every > > dquot in the buffer. Remove the duplicate call from > > xfs_qm_dqflush(). > > Ah, I think there's a bug here - it's not supposed to be a > duplicate.... > > > Signed-off-by: Brian Foster <bfoster@redhat.com> > > --- > > fs/xfs/xfs_dquot.c | 14 -------------- > > 1 file changed, 14 deletions(-) > > > > diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c > > index af2c8e5ceea0..73032c18a94a 100644 > > --- a/fs/xfs/xfs_dquot.c > > +++ b/fs/xfs/xfs_dquot.c > > @@ -1071,7 +1071,6 @@ xfs_qm_dqflush( > > struct xfs_buf *bp; > > struct xfs_dqblk *dqb; > > struct xfs_disk_dquot *ddqp; > > - xfs_failaddr_t fa; > > int error; > > > > ASSERT(XFS_DQ_IS_LOCKED(dqp)); > > @@ -1116,19 +1115,6 @@ xfs_qm_dqflush( > > dqb = bp->b_addr + dqp->q_bufoffset; > > ddqp = &dqb->dd_diskdq; > > > > - /* > > - * A simple sanity check in case we got a corrupted dquot. > > - */ > > - fa = xfs_dqblk_verify(mp, dqb, be32_to_cpu(ddqp->d_id), 0); > > So this verifies the on disk dquot .... > > > - if (fa) { > > - xfs_alert(mp, "corrupt dquot ID 0x%x in memory at %pS", > > ...which issues an "in memory corruption" alert on failure... > > > - be32_to_cpu(ddqp->d_id), fa); > > - xfs_buf_relse(bp); > > - xfs_dqfunlock(dqp); > > - xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE); > > - return -EFSCORRUPTED; > > - } > > - > > /* This is the only portion of data that needs to persist */ > > memcpy(ddqp, &dqp->q_core, sizeof(struct xfs_disk_dquot)); > > .... and on success we immediately overwrite the on-disk copy with > the unchecked in-memory copy of the dquot. > > IOWs, I think that verification call here should be checking the > in-memory dquot core, not the on disk buffer that is about to get > trashed. i.e. something like this: > > - fa = xfs_dqblk_verify(mp, dqb, be32_to_cpu(ddqp->d_id), 0); > + fa = xfs_dquot_verify(mp, &dqp->q_core, be32_to_cpu(ddqp->d_id), 0); > Isn't this still essentially duplicated by the write verifier? I don't feel strongly about changing it as above vs. removing it, but it does still seem unnecessary to me.. Brian > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com >
On Mon, Apr 20, 2020 at 10:02:21AM -0400, Brian Foster wrote: > On Mon, Apr 20, 2020 at 01:53:22PM +1000, Dave Chinner wrote: > > On Fri, Apr 17, 2020 at 11:08:53AM -0400, Brian Foster wrote: > > > The dquot read/write verifier calls xfs_dqblk_verify() on every > > > dquot in the buffer. Remove the duplicate call from > > > xfs_qm_dqflush(). > > > > Ah, I think there's a bug here - it's not supposed to be a > > duplicate.... > > > > > Signed-off-by: Brian Foster <bfoster@redhat.com> > > > --- > > > fs/xfs/xfs_dquot.c | 14 -------------- > > > 1 file changed, 14 deletions(-) > > > > > > diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c > > > index af2c8e5ceea0..73032c18a94a 100644 > > > --- a/fs/xfs/xfs_dquot.c > > > +++ b/fs/xfs/xfs_dquot.c > > > @@ -1071,7 +1071,6 @@ xfs_qm_dqflush( > > > struct xfs_buf *bp; > > > struct xfs_dqblk *dqb; > > > struct xfs_disk_dquot *ddqp; > > > - xfs_failaddr_t fa; > > > int error; > > > > > > ASSERT(XFS_DQ_IS_LOCKED(dqp)); > > > @@ -1116,19 +1115,6 @@ xfs_qm_dqflush( > > > dqb = bp->b_addr + dqp->q_bufoffset; > > > ddqp = &dqb->dd_diskdq; > > > > > > - /* > > > - * A simple sanity check in case we got a corrupted dquot. > > > - */ > > > - fa = xfs_dqblk_verify(mp, dqb, be32_to_cpu(ddqp->d_id), 0); > > > > So this verifies the on disk dquot .... > > > > > - if (fa) { > > > - xfs_alert(mp, "corrupt dquot ID 0x%x in memory at %pS", > > > > ...which issues an "in memory corruption" alert on failure... > > > > > - be32_to_cpu(ddqp->d_id), fa); > > > - xfs_buf_relse(bp); > > > - xfs_dqfunlock(dqp); > > > - xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE); > > > - return -EFSCORRUPTED; > > > - } > > > - > > > /* This is the only portion of data that needs to persist */ > > > memcpy(ddqp, &dqp->q_core, sizeof(struct xfs_disk_dquot)); > > > > .... and on success we immediately overwrite the on-disk copy with > > the unchecked in-memory copy of the dquot. > > > > IOWs, I think that verification call here should be checking the > > in-memory dquot core, not the on disk buffer that is about to get > > trashed. i.e. something like this: > > > > - fa = xfs_dqblk_verify(mp, dqb, be32_to_cpu(ddqp->d_id), 0); > > + fa = xfs_dquot_verify(mp, &dqp->q_core, be32_to_cpu(ddqp->d_id), 0); > > > > Isn't this still essentially duplicated by the write verifier? I don't > feel strongly about changing it as above vs. removing it, but it does > still seem unnecessary to me.. It's no different to the xfs_iflush_int() code that runs a heap of checks on the in-memory inode before it is flushed to the backing buffer. That uses a combination of open coded checks (for error injection) and verifier functions (e.g. fork checking), so this really isn't that unusual. Realistically, it's better to catch the corruption as early as possible - if we catch it here we know we corrupted the in-memory dquot. However, if the write verifier catches it we have no idea exactly when the corruption occurred, or whether it was a result of a code problem or an external memory corruption in memory we haven't modified at all... IOWs the two checks or intended to catch very different classes of in-memory corruptions, so they really aren't redundant or unnecessary at all... Cheers, Dave.
diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c index af2c8e5ceea0..73032c18a94a 100644 --- a/fs/xfs/xfs_dquot.c +++ b/fs/xfs/xfs_dquot.c @@ -1071,7 +1071,6 @@ xfs_qm_dqflush( struct xfs_buf *bp; struct xfs_dqblk *dqb; struct xfs_disk_dquot *ddqp; - xfs_failaddr_t fa; int error; ASSERT(XFS_DQ_IS_LOCKED(dqp)); @@ -1116,19 +1115,6 @@ xfs_qm_dqflush( dqb = bp->b_addr + dqp->q_bufoffset; ddqp = &dqb->dd_diskdq; - /* - * A simple sanity check in case we got a corrupted dquot. - */ - fa = xfs_dqblk_verify(mp, dqb, be32_to_cpu(ddqp->d_id), 0); - if (fa) { - xfs_alert(mp, "corrupt dquot ID 0x%x in memory at %pS", - be32_to_cpu(ddqp->d_id), fa); - xfs_buf_relse(bp); - xfs_dqfunlock(dqp); - xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE); - return -EFSCORRUPTED; - } - /* This is the only portion of data that needs to persist */ memcpy(ddqp, &dqp->q_core, sizeof(struct xfs_disk_dquot));
The dquot read/write verifier calls xfs_dqblk_verify() on every dquot in the buffer. Remove the duplicate call from xfs_qm_dqflush(). Signed-off-by: Brian Foster <bfoster@redhat.com> --- fs/xfs/xfs_dquot.c | 14 -------------- 1 file changed, 14 deletions(-)