* [PATCH] xfs_repair: fix max block offset test @ 2014-12-11 17:54 Eric Sandeen 2014-12-18 15:18 ` Brian Foster 2014-12-19 19:58 ` [PATCH V2] " Eric Sandeen 0 siblings, 2 replies; 7+ messages in thread From: Eric Sandeen @ 2014-12-11 17:54 UTC (permalink / raw) To: xfs-oss; +Cc: Eryu Guan Eryu pointed out that in fstest xfs/071, we find corruption reported at the end. This test attempts to do IO at the maximum possible offsets, and repair yields: inode 1027 - extent offset too large - start 70, count 1, offset 2251799813685247 correcting nextents for inode 1027 bad data fork in inode 1027 would have cleared inode 1027 Repair is complaining that an extent *starts* at the maximum block, but AFAICT, starting there is just fine, as long as we also end there. i.e. a one-block extent at the limit is just fine. So change the xfs_repair test to allow this situation. Reported-by: Eryu Guan <eguan@redhat.com> Signed-off-by: Eric Sandeen <sandeen@redhat.com> --- diff --git a/repair/dinode.c b/repair/dinode.c index 38a6562..ca57a61 100644 --- a/repair/dinode.c +++ b/repair/dinode.c @@ -667,7 +667,9 @@ _("inode %" PRIu64 " - bad extent overflows - start %" PRIu64 ", " irec.br_startoff); goto done; } - if (irec.br_startoff >= fs_max_file_offset) { + /* Ensure this extent does not extend beyond the max offset */ + if (irec.br_startoff + irec.br_blockcount - 1 > + fs_max_file_offset) { do_warn( _("inode %" PRIu64 " - extent offset too large - start %" PRIu64 ", " "count %" PRIu64 ", offset %" PRIu64 "\n"), _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] xfs_repair: fix max block offset test 2014-12-11 17:54 [PATCH] xfs_repair: fix max block offset test Eric Sandeen @ 2014-12-18 15:18 ` Brian Foster 2014-12-18 16:43 ` Eric Sandeen 2014-12-19 18:54 ` Eric Sandeen 2014-12-19 19:58 ` [PATCH V2] " Eric Sandeen 1 sibling, 2 replies; 7+ messages in thread From: Brian Foster @ 2014-12-18 15:18 UTC (permalink / raw) To: Eric Sandeen; +Cc: Eryu Guan, xfs-oss On Thu, Dec 11, 2014 at 11:54:03AM -0600, Eric Sandeen wrote: > Eryu pointed out that in fstest xfs/071, we find corruption > reported at the end. This test attempts to do IO at the > maximum possible offsets, and repair yields: > > inode 1027 - extent offset too large - start 70, count 1, offset 2251799813685247 > correcting nextents for inode 1027 > bad data fork in inode 1027 > would have cleared inode 1027 > > Repair is complaining that an extent *starts* at the maximum > block, but AFAICT, starting there is just fine, as long as > we also end there. i.e. a one-block extent at the limit > is just fine. > > So change the xfs_repair test to allow this situation. > > Reported-by: Eryu Guan <eguan@redhat.com> > Signed-off-by: Eric Sandeen <sandeen@redhat.com> > --- Thought this looked familiar: http://oss.sgi.com/archives/xfs/2014-09/msg00524.html Either one is fine with me. If we use the fix below, I think we should update the error message since it technically refers to the extent offset and we slightly tweak the meaning of the failure. Brian > > diff --git a/repair/dinode.c b/repair/dinode.c > index 38a6562..ca57a61 100644 > --- a/repair/dinode.c > +++ b/repair/dinode.c > @@ -667,7 +667,9 @@ _("inode %" PRIu64 " - bad extent overflows - start %" PRIu64 ", " > irec.br_startoff); > goto done; > } > - if (irec.br_startoff >= fs_max_file_offset) { > + /* Ensure this extent does not extend beyond the max offset */ > + if (irec.br_startoff + irec.br_blockcount - 1 > > + fs_max_file_offset) { > do_warn( > _("inode %" PRIu64 " - extent offset too large - start %" PRIu64 ", " > "count %" PRIu64 ", offset %" PRIu64 "\n"), > > _______________________________________________ > xfs mailing list > xfs@oss.sgi.com > http://oss.sgi.com/mailman/listinfo/xfs _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] xfs_repair: fix max block offset test 2014-12-18 15:18 ` Brian Foster @ 2014-12-18 16:43 ` Eric Sandeen 2014-12-19 18:54 ` Eric Sandeen 1 sibling, 0 replies; 7+ messages in thread From: Eric Sandeen @ 2014-12-18 16:43 UTC (permalink / raw) To: Brian Foster; +Cc: Eryu Guan, xfs-oss On 12/18/14 9:18 AM, Brian Foster wrote: > Thought this looked familiar: > > http://oss.sgi.com/archives/xfs/2014-09/msg00524.html oh, whoops. let me go read that, sorry! -Eric > Either one is fine with me. If we use the fix below, I think we should > update the error message since it technically refers to the extent > offset and we slightly tweak the meaning of the failure. > > Brian > >> >> diff --git a/repair/dinode.c b/repair/dinode.c >> index 38a6562..ca57a61 100644 >> --- a/repair/dinode.c >> +++ b/repair/dinode.c >> @@ -667,7 +667,9 @@ _("inode %" PRIu64 " - bad extent overflows - start %" PRIu64 ", " >> irec.br_startoff); >> goto done; >> } >> - if (irec.br_startoff >= fs_max_file_offset) { >> + /* Ensure this extent does not extend beyond the max offset */ >> + if (irec.br_startoff + irec.br_blockcount - 1 > >> + fs_max_file_offset) { >> do_warn( >> _("inode %" PRIu64 " - extent offset too large - start %" PRIu64 ", " >> "count %" PRIu64 ", offset %" PRIu64 "\n"), >> >> _______________________________________________ >> xfs mailing list >> xfs@oss.sgi.com >> http://oss.sgi.com/mailman/listinfo/xfs _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] xfs_repair: fix max block offset test 2014-12-18 15:18 ` Brian Foster 2014-12-18 16:43 ` Eric Sandeen @ 2014-12-19 18:54 ` Eric Sandeen 2014-12-19 20:26 ` Brian Foster 1 sibling, 1 reply; 7+ messages in thread From: Eric Sandeen @ 2014-12-19 18:54 UTC (permalink / raw) To: Brian Foster, Eric Sandeen; +Cc: Eryu Guan, xfs-oss On 12/18/14 9:18 AM, Brian Foster wrote: hought this looked familiar: > > http://oss.sgi.com/archives/xfs/2014-09/msg00524.html > > Either one is fine with me. If we use the fix below, I think we should > update the error message since it technically refers to the extent > offset and we slightly tweak the meaning of the failure. > > Brian Ok, so here's what I wonder about with your patch. + if (irec.br_startoff > fs_max_file_offset) { What if the extent starting logical offset is *at* fs_max_file_offset (which is sadly in blocks, not bytes, for extra confusion points), but the extent is 1GB in length? Isn't this a problem because we've now got files in the block which are beyond fs_max_file_offset, and probably also beyond sb_maxbytes? I'm not sure I follow why you feel the message needs to change with my version: + /* Ensure this extent does not extend beyond the max offset */ + if (irec.br_startoff + irec.br_blockcount - 1 > + fs_max_file_offset) { do_warn( _("inode %" PRIu64 " - extent offset too large - start %" PRIu64 ", " "count %" PRIu64 ", offset %" PRIu64 "\n"), ino, irec.br_startblock, irec.br_blockcount, irec.br_startoff); If any (block) offset in the file is beyond the max, then we say the offset is too large, and give it's physical block, its length, and its starting offset (granted, the message is a bit *weird* - br_startblock really doesn't matter here, but I don't think I've fundamentally changed the meaning of the test). However, I've come to learn that you are almost always right about subtleties like this, so perhaps you can explain again to me, using smaller words. :) -Eric _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] xfs_repair: fix max block offset test 2014-12-19 18:54 ` Eric Sandeen @ 2014-12-19 20:26 ` Brian Foster 0 siblings, 0 replies; 7+ messages in thread From: Brian Foster @ 2014-12-19 20:26 UTC (permalink / raw) To: Eric Sandeen; +Cc: Eric Sandeen, Eryu Guan, xfs-oss On Fri, Dec 19, 2014 at 12:54:22PM -0600, Eric Sandeen wrote: > On 12/18/14 9:18 AM, Brian Foster wrote: > > hought this looked familiar: > > > > http://oss.sgi.com/archives/xfs/2014-09/msg00524.html > > > > Either one is fine with me. If we use the fix below, I think we should > > update the error message since it technically refers to the extent > > offset and we slightly tweak the meaning of the failure. > > > > Brian > > Ok, so here's what I wonder about with your patch. > > + if (irec.br_startoff > fs_max_file_offset) { > > What if the extent starting logical offset is *at* fs_max_file_offset > (which is sadly in blocks, not bytes, for extra confusion points), > but the extent is 1GB in length? > > Isn't this a problem because we've now got files in the block which > are beyond fs_max_file_offset, and probably also beyond sb_maxbytes? > Yeah... at the time I simply viewed this as outside the scope of this particular check due to the logic and wording. It makes sense to widen the logic to catch anything that starts before yet extends past max offset, assuming we aren't actually checking that anywhere else. > I'm not sure I follow why you feel the message needs to change with > my version: > > + /* Ensure this extent does not extend beyond the max offset */ > + if (irec.br_startoff + irec.br_blockcount - 1 > > + fs_max_file_offset) { > do_warn( > _("inode %" PRIu64 " - extent offset too large - start %" PRIu64 ", " > "count %" PRIu64 ", offset %" PRIu64 "\n"), > ino, irec.br_startblock, irec.br_blockcount, > irec.br_startoff); > > If any (block) offset in the file is beyond the max, then we say the offset is > too large, and give it's physical block, its length, and its starting offset > (granted, the message is a bit *weird* - br_startblock really doesn't matter > here, but I don't think I've fundamentally changed the meaning of the test). As discussed on irc, I read "extent offset" as the offset of the extent in the file, so it seemed a bit confusing if br_startoff isn't necessarily the problem. Brian > > However, I've come to learn that you are almost always right about subtleties > like this, so perhaps you can explain again to me, using smaller words. :) > > -Eric > > _______________________________________________ > xfs mailing list > xfs@oss.sgi.com > http://oss.sgi.com/mailman/listinfo/xfs _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH V2] xfs_repair: fix max block offset test 2014-12-11 17:54 [PATCH] xfs_repair: fix max block offset test Eric Sandeen 2014-12-18 15:18 ` Brian Foster @ 2014-12-19 19:58 ` Eric Sandeen 2014-12-19 20:31 ` Brian Foster 1 sibling, 1 reply; 7+ messages in thread From: Eric Sandeen @ 2014-12-19 19:58 UTC (permalink / raw) To: Eric Sandeen, xfs-oss; +Cc: Eryu Guan Eryu pointed out that in fstest xfs/071, we find corruption reported at the end. This test attempts to do IO at the maximum possible offsets, and repair yields: inode 1027 - extent offset too large - start 70, count 1, offset 2251799813685247 correcting nextents for inode 1027 bad data fork in inode 1027 would have cleared inode 1027 Repair is complaining that an extent *starts* at the maximum block, but AFAICT, starting there is just fine, as long as we also end there. i.e. a one-block extent at the limit is just fine. So change the xfs_repair test to allow this situation. Also, the warning text is a bit unclear, mixing in the physical block w/ the logical block... rearrange that a little to make it obvious. Reported-by: Eryu Guan <eguan@redhat.com> Signed-off-by: Eric Sandeen <sandeen@redhat.com> --- V2: Update the warning text diff --git a/repair/dinode.c b/repair/dinode.c index 38a6562..59824ec 100644 --- a/repair/dinode.c +++ b/repair/dinode.c @@ -667,12 +667,14 @@ _("inode %" PRIu64 " - bad extent overflows - start %" PRIu64 ", " irec.br_startoff); goto done; } - if (irec.br_startoff >= fs_max_file_offset) { + /* Ensure this extent does not extend beyond the max offset */ + if (irec.br_startoff + irec.br_blockcount - 1 > + fs_max_file_offset) { do_warn( -_("inode %" PRIu64 " - extent offset too large - start %" PRIu64 ", " - "count %" PRIu64 ", offset %" PRIu64 "\n"), - ino, irec.br_startblock, irec.br_blockcount, - irec.br_startoff); +_("inode %" PRIu64 " - extent exceeds max offset - start %" PRIu64 ", " + "count %" PRIu64 ", physical block %" PRIu64 "\n"), + ino, irec.br_startoff, irec.br_blockcount, + irec.br_startblock); goto done; } _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH V2] xfs_repair: fix max block offset test 2014-12-19 19:58 ` [PATCH V2] " Eric Sandeen @ 2014-12-19 20:31 ` Brian Foster 0 siblings, 0 replies; 7+ messages in thread From: Brian Foster @ 2014-12-19 20:31 UTC (permalink / raw) To: Eric Sandeen; +Cc: Eric Sandeen, Eryu Guan, xfs-oss On Fri, Dec 19, 2014 at 01:58:26PM -0600, Eric Sandeen wrote: > Eryu pointed out that in fstest xfs/071, we find corruption > reported at the end. This test attempts to do IO at the > maximum possible offsets, and repair yields: > > inode 1027 - extent offset too large - start 70, count 1, offset 2251799813685247 > correcting nextents for inode 1027 > bad data fork in inode 1027 > would have cleared inode 1027 > > Repair is complaining that an extent *starts* at the maximum > block, but AFAICT, starting there is just fine, as long as > we also end there. i.e. a one-block extent at the limit > is just fine. > > So change the xfs_repair test to allow this situation. > > Also, the warning text is a bit unclear, mixing in the physical > block w/ the logical block... rearrange that a little to make > it obvious. > > Reported-by: Eryu Guan <eguan@redhat.com> > Signed-off-by: Eric Sandeen <sandeen@redhat.com> > --- Looks good to me. Thanks for the fixup. :) Reviewed-by: Brian Foster <bfoster@redhat.com> > > V2: Update the warning text > > diff --git a/repair/dinode.c b/repair/dinode.c > index 38a6562..59824ec 100644 > --- a/repair/dinode.c > +++ b/repair/dinode.c > @@ -667,12 +667,14 @@ _("inode %" PRIu64 " - bad extent overflows - start %" PRIu64 ", " > irec.br_startoff); > goto done; > } > - if (irec.br_startoff >= fs_max_file_offset) { > + /* Ensure this extent does not extend beyond the max offset */ > + if (irec.br_startoff + irec.br_blockcount - 1 > > + fs_max_file_offset) { > do_warn( > -_("inode %" PRIu64 " - extent offset too large - start %" PRIu64 ", " > - "count %" PRIu64 ", offset %" PRIu64 "\n"), > - ino, irec.br_startblock, irec.br_blockcount, > - irec.br_startoff); > +_("inode %" PRIu64 " - extent exceeds max offset - start %" PRIu64 ", " > + "count %" PRIu64 ", physical block %" PRIu64 "\n"), > + ino, irec.br_startoff, irec.br_blockcount, > + irec.br_startblock); > goto done; > } > > > > _______________________________________________ > xfs mailing list > xfs@oss.sgi.com > http://oss.sgi.com/mailman/listinfo/xfs _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-12-19 20:31 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-12-11 17:54 [PATCH] xfs_repair: fix max block offset test Eric Sandeen 2014-12-18 15:18 ` Brian Foster 2014-12-18 16:43 ` Eric Sandeen 2014-12-19 18:54 ` Eric Sandeen 2014-12-19 20:26 ` Brian Foster 2014-12-19 19:58 ` [PATCH V2] " Eric Sandeen 2014-12-19 20:31 ` Brian Foster
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox