| Message ID | 168506055189.3727958.722711918040129046.stgit@frogsfrogsfrogs (mailing list archive) |
|---|---|
| Headers | show |
| Series | xfs: fix ranged queries and integer overflows in GETFSMAP | expand |
Ping? Even if it's too late for 6.5, can we at least get this bug fix series reviewed? --D On Thu, May 25, 2023 at 05:28:08PM -0700, Darrick J. Wong wrote: > Hi all, > > As part of merging the parent pointers patchset into my development > branch, I noticed a few problems with the GETFSMAP implementation for > XFS. The biggest problem is that ranged queries don't work properly if > the query interval is exactly within a single record. It turns out that > I didn't implement the record filtering quite right -- for the first > call into the btree code, we want to find any rmap that overlaps with > the range specified, but for subsequent calls, we only want rmaps that > come after the low key of the query. This can be fixed by tweaking the > filtering logic and pushing the key handling into the individual backend > implementations. > > The second problem I noticed is that there are integer overflows in the > rtbitmap and external log handlers. This is the result of a poor > decision on my part to use the incore rmap records for storing the query > ranges; this only works for the rmap code, which is smart enough to > iterate AGs. This breaks down spectacularly if someone tries to query > high block offsets in either the rt volume or the log device. I fixed > that by introducing a second filtering implementation based entirely on > daddrs. > > The third problem was minor by comparison -- the rt volume cannot ever > use rtblocks beyond the end of the last rtextent, so it makes no sense > for GETFSMAP to try to query those areas. > > Having done that, add a few more patches to clean up some messes. > > If you're going to start using this mess, you probably ought to just > pull from my git trees, which are linked below. > > This is an extraordinary way to destroy everything. Enjoy! > Comments and questions are, as always, welcome. > > --D > > kernel git tree: > https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=getfsmap-fixes > > xfsprogs git tree: > https://git.kernel.org/cgit/linux/kernel/git/djwong/xfsprogs-dev.git/log/?h=getfsmap-fixes > --- > fs/xfs/libxfs/xfs_alloc.c | 10 -- > fs/xfs/libxfs/xfs_refcount.c | 13 +- > fs/xfs/libxfs/xfs_rmap.c | 10 -- > fs/xfs/xfs_fsmap.c | 261 ++++++++++++++++++++++-------------------- > fs/xfs/xfs_trace.h | 25 ++++ > 5 files changed, 177 insertions(+), 142 deletions(-) >
On Sun, Jun 04, 2023 at 10:56:23AM -0700, Darrick J. Wong wrote: > Ping? Even if it's too late for 6.5, can we at least get this bug fix > series reviewed? I haven't even had time to read the description of this mega patchset, so I had no idea there were bug fixes in it. Normally you send them separately, so it's obvious they are bug fixes that need immediate attention. I'll put it on my list, but I have no idea right now when I'll get to it.... Cheers, Dave.
On Thu, May 25, 2023 at 05:28:08PM -0700, Darrick J. Wong wrote: > Hi all, > > As part of merging the parent pointers patchset into my development > branch, I noticed a few problems with the GETFSMAP implementation for > XFS. The biggest problem is that ranged queries don't work properly if > the query interval is exactly within a single record. It turns out that > I didn't implement the record filtering quite right -- for the first > call into the btree code, we want to find any rmap that overlaps with > the range specified, but for subsequent calls, we only want rmaps that > come after the low key of the query. This can be fixed by tweaking the > filtering logic and pushing the key handling into the individual backend > implementations. > > The second problem I noticed is that there are integer overflows in the > rtbitmap and external log handlers. This is the result of a poor > decision on my part to use the incore rmap records for storing the query > ranges; this only works for the rmap code, which is smart enough to > iterate AGs. This breaks down spectacularly if someone tries to query > high block offsets in either the rt volume or the log device. I fixed > that by introducing a second filtering implementation based entirely on > daddrs. > > The third problem was minor by comparison -- the rt volume cannot ever > use rtblocks beyond the end of the last rtextent, so it makes no sense > for GETFSMAP to try to query those areas. > > Having done that, add a few more patches to clean up some messes. > > If you're going to start using this mess, you probably ought to just > pull from my git trees, which are linked below. > > This is an extraordinary way to destroy everything. Enjoy! > Comments and questions are, as always, welcome. > > --D > > kernel git tree: > https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=getfsmap-fixes > > xfsprogs git tree: > https://git.kernel.org/cgit/linux/kernel/git/djwong/xfsprogs-dev.git/log/?h=getfsmap-fixes > --- > fs/xfs/libxfs/xfs_alloc.c | 10 -- > fs/xfs/libxfs/xfs_refcount.c | 13 +- > fs/xfs/libxfs/xfs_rmap.c | 10 -- > fs/xfs/xfs_fsmap.c | 261 ++++++++++++++++++++++-------------------- > fs/xfs/xfs_trace.h | 25 ++++ > 5 files changed, 177 insertions(+), 142 deletions(-) Changes look sensible, but I know my limits: I'm not going to find off-by-one problems in this code during review. Reviewed-by: Dave Chinner <dchinner@redhat.com>
On Tue, Jun 20, 2023 at 12:09:39PM +1000, Dave Chinner wrote: > On Thu, May 25, 2023 at 05:28:08PM -0700, Darrick J. Wong wrote: > > Hi all, > > > > As part of merging the parent pointers patchset into my development > > branch, I noticed a few problems with the GETFSMAP implementation for > > XFS. The biggest problem is that ranged queries don't work properly if > > the query interval is exactly within a single record. It turns out that > > I didn't implement the record filtering quite right -- for the first > > call into the btree code, we want to find any rmap that overlaps with > > the range specified, but for subsequent calls, we only want rmaps that > > come after the low key of the query. This can be fixed by tweaking the > > filtering logic and pushing the key handling into the individual backend > > implementations. > > > > The second problem I noticed is that there are integer overflows in the > > rtbitmap and external log handlers. This is the result of a poor > > decision on my part to use the incore rmap records for storing the query > > ranges; this only works for the rmap code, which is smart enough to > > iterate AGs. This breaks down spectacularly if someone tries to query > > high block offsets in either the rt volume or the log device. I fixed > > that by introducing a second filtering implementation based entirely on > > daddrs. > > > > The third problem was minor by comparison -- the rt volume cannot ever > > use rtblocks beyond the end of the last rtextent, so it makes no sense > > for GETFSMAP to try to query those areas. > > > > Having done that, add a few more patches to clean up some messes. > > > > If you're going to start using this mess, you probably ought to just > > pull from my git trees, which are linked below. > > > > This is an extraordinary way to destroy everything. Enjoy! > > Comments and questions are, as always, welcome. > > > > --D > > > > kernel git tree: > > https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=getfsmap-fixes > > > > xfsprogs git tree: > > https://git.kernel.org/cgit/linux/kernel/git/djwong/xfsprogs-dev.git/log/?h=getfsmap-fixes > > --- > > fs/xfs/libxfs/xfs_alloc.c | 10 -- > > fs/xfs/libxfs/xfs_refcount.c | 13 +- > > fs/xfs/libxfs/xfs_rmap.c | 10 -- > > fs/xfs/xfs_fsmap.c | 261 ++++++++++++++++++++++-------------------- > > fs/xfs/xfs_trace.h | 25 ++++ > > 5 files changed, 177 insertions(+), 142 deletions(-) > > Changes look sensible, but I know my limits: I'm not going to find > off-by-one problems in this code during review. > > Reviewed-by: Dave Chinner <dchinner@redhat.com> Oh, heh, thanks for the review! I guess I'll go spin the for-next bottle again... --D > -- > Dave Chinner > david@fromorbit.com