| Message ID | 20200510072404.986627-2-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/6] xfs: use XFS_IFORK_BOFF xchk_bmap_check_rmaps | expand |
On Sunday 10 May 2020 12:53:59 PM IST Christoph Hellwig wrote: > XFS_IFORK_Q is to be used in boolean context, not for a size. This > doesn't make a difference in practice as size is only checked for > 0, but this keeps the logic sane. > Wouldn't XFS_IFORK_ASIZE() be a better fit since it gives the space used by the attr fork inside an inode's literal area? > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/xfs/scrub/bmap.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/xfs/scrub/bmap.c b/fs/xfs/scrub/bmap.c > index add8598eacd5d..283424d6d2bb6 100644 > --- a/fs/xfs/scrub/bmap.c > +++ b/fs/xfs/scrub/bmap.c > @@ -591,7 +591,7 @@ xchk_bmap_check_rmaps( > size = i_size_read(VFS_I(sc->ip)); > break; > case XFS_ATTR_FORK: > - size = XFS_IFORK_Q(sc->ip); > + size = XFS_IFORK_BOFF(sc->ip); > break; > default: > size = 0; >
On Mon, May 11, 2020 at 05:10:04PM +0530, Chandan Babu R wrote: > On Sunday 10 May 2020 12:53:59 PM IST Christoph Hellwig wrote: > > XFS_IFORK_Q is to be used in boolean context, not for a size. This > > doesn't make a difference in practice as size is only checked for > > 0, but this keeps the logic sane. > > > > Wouldn't XFS_IFORK_ASIZE() be a better fit since it gives the space used by the > attr fork inside an inode's literal area? > I had the same thought. It's not clear to me what size is really supposed to be between the file size for a data fork and fork offset for the attr fork. I was also wondering if this should use XFS_IFORK_DSIZE(), but that won't be conditional based on population of the fork. At the same time, I don't think i_size != 0 necessarily correlates with the existence of blocks. The file could be completely sparse or could have any number of post-eof preallocated extents. Brian > > Signed-off-by: Christoph Hellwig <hch@lst.de> > > --- > > fs/xfs/scrub/bmap.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/fs/xfs/scrub/bmap.c b/fs/xfs/scrub/bmap.c > > index add8598eacd5d..283424d6d2bb6 100644 > > --- a/fs/xfs/scrub/bmap.c > > +++ b/fs/xfs/scrub/bmap.c > > @@ -591,7 +591,7 @@ xchk_bmap_check_rmaps( > > size = i_size_read(VFS_I(sc->ip)); > > break; > > case XFS_ATTR_FORK: > > - size = XFS_IFORK_Q(sc->ip); > > + size = XFS_IFORK_BOFF(sc->ip); > > break; > > default: > > size = 0; > > > > > -- > chandan > > >
On Tue, May 12, 2020 at 11:31:32AM -0400, Brian Foster wrote: > On Mon, May 11, 2020 at 05:10:04PM +0530, Chandan Babu R wrote: > > On Sunday 10 May 2020 12:53:59 PM IST Christoph Hellwig wrote: > > > XFS_IFORK_Q is to be used in boolean context, not for a size. This > > > doesn't make a difference in practice as size is only checked for > > > 0, but this keeps the logic sane. > > > > > > > Wouldn't XFS_IFORK_ASIZE() be a better fit since it gives the space used by the > > attr fork inside an inode's literal area? > > > > I had the same thought. It's not clear to me what size is really > supposed to be between the file size for a data fork and fork offset for > the attr fork. I was also wondering if this should use > XFS_IFORK_DSIZE(), but that won't be conditional based on population of > the fork. At the same time, I don't think i_size != 0 necessarily > correlates with the existence of blocks. The file could be completely > sparse or could have any number of post-eof preallocated extents. TBH I should have made that variable "bool empty" or something. case XFS_DATA_FORK: empty = i_size_read() == 0; case XFS_ATTR_FORK: empty = !XFS_IFORK_Q(); default: empty = true; if ((is not btree) && (empty || nextents > 0)) return 0; --D > Brian > > > > Signed-off-by: Christoph Hellwig <hch@lst.de> > > > --- > > > fs/xfs/scrub/bmap.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/fs/xfs/scrub/bmap.c b/fs/xfs/scrub/bmap.c > > > index add8598eacd5d..283424d6d2bb6 100644 > > > --- a/fs/xfs/scrub/bmap.c > > > +++ b/fs/xfs/scrub/bmap.c > > > @@ -591,7 +591,7 @@ xchk_bmap_check_rmaps( > > > size = i_size_read(VFS_I(sc->ip)); > > > break; > > > case XFS_ATTR_FORK: > > > - size = XFS_IFORK_Q(sc->ip); > > > + size = XFS_IFORK_BOFF(sc->ip); > > > break; > > > default: > > > size = 0; > > > > > > > > > -- > > chandan > > > > > > >
On Tue, May 12, 2020 at 08:38:54AM -0700, Darrick J. Wong wrote: > On Tue, May 12, 2020 at 11:31:32AM -0400, Brian Foster wrote: > > On Mon, May 11, 2020 at 05:10:04PM +0530, Chandan Babu R wrote: > > > On Sunday 10 May 2020 12:53:59 PM IST Christoph Hellwig wrote: > > > > XFS_IFORK_Q is to be used in boolean context, not for a size. This > > > > doesn't make a difference in practice as size is only checked for > > > > 0, but this keeps the logic sane. > > > > > > > > > > Wouldn't XFS_IFORK_ASIZE() be a better fit since it gives the space used by the > > > attr fork inside an inode's literal area? > > > > > > > I had the same thought. It's not clear to me what size is really > > supposed to be between the file size for a data fork and fork offset for > > the attr fork. I was also wondering if this should use > > XFS_IFORK_DSIZE(), but that won't be conditional based on population of > > the fork. At the same time, I don't think i_size != 0 necessarily > > correlates with the existence of blocks. The file could be completely > > sparse or could have any number of post-eof preallocated extents. > > TBH I should have made that variable "bool empty" or something. > > case XFS_DATA_FORK: > empty = i_size_read() == 0; > Even that is somewhat unclear because it's tied to i_size. What about size == 0 && <post-eof extents>? Brian > case XFS_ATTR_FORK: > empty = !XFS_IFORK_Q(); > > default: > empty = true; > > if ((is not btree) && (empty || nextents > 0)) > return 0; > > --D > > > Brian > > > > > > Signed-off-by: Christoph Hellwig <hch@lst.de> > > > > --- > > > > fs/xfs/scrub/bmap.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/fs/xfs/scrub/bmap.c b/fs/xfs/scrub/bmap.c > > > > index add8598eacd5d..283424d6d2bb6 100644 > > > > --- a/fs/xfs/scrub/bmap.c > > > > +++ b/fs/xfs/scrub/bmap.c > > > > @@ -591,7 +591,7 @@ xchk_bmap_check_rmaps( > > > > size = i_size_read(VFS_I(sc->ip)); > > > > break; > > > > case XFS_ATTR_FORK: > > > > - size = XFS_IFORK_Q(sc->ip); > > > > + size = XFS_IFORK_BOFF(sc->ip); > > > > break; > > > > default: > > > > size = 0; > > > > > > > > > > > > > -- > > > chandan > > > > > > > > > > > >
On Tue, May 12, 2020 at 12:14:10PM -0400, Brian Foster wrote: > On Tue, May 12, 2020 at 08:38:54AM -0700, Darrick J. Wong wrote: > > On Tue, May 12, 2020 at 11:31:32AM -0400, Brian Foster wrote: > > > On Mon, May 11, 2020 at 05:10:04PM +0530, Chandan Babu R wrote: > > > > On Sunday 10 May 2020 12:53:59 PM IST Christoph Hellwig wrote: > > > > > XFS_IFORK_Q is to be used in boolean context, not for a size. This > > > > > doesn't make a difference in practice as size is only checked for > > > > > 0, but this keeps the logic sane. > > > > > > > > > > > > > Wouldn't XFS_IFORK_ASIZE() be a better fit since it gives the space used by the > > > > attr fork inside an inode's literal area? > > > > > > > > > > I had the same thought. It's not clear to me what size is really > > > supposed to be between the file size for a data fork and fork offset for > > > the attr fork. I was also wondering if this should use > > > XFS_IFORK_DSIZE(), but that won't be conditional based on population of > > > the fork. At the same time, I don't think i_size != 0 necessarily > > > correlates with the existence of blocks. The file could be completely > > > sparse or could have any number of post-eof preallocated extents. > > > > TBH I should have made that variable "bool empty" or something. > > > > case XFS_DATA_FORK: > > empty = i_size_read() == 0; > > > > Even that is somewhat unclear because it's tied to i_size. What about > size == 0 && <post-eof extents>? /me stumbles around trying to remember under what circumstances do we actually scan all the rmaps to make sure there's a corresponding bmap. I think we're trying to avoid doing it for every file in the filesystem because it's very slow... Oh right -- we do that for btree format forks because there are a lot of extent records and therefore a high(er) chance of something getting lost; or We scan all the rmaps for files that have nonzero size but zero extents, because the (forthcoming) inode fork repair will reset damaged forks back to extents format with zero extents, and this is how we will trigger the bmap repair to get the extents mapped back into the file. --D > > Brian > > > case XFS_ATTR_FORK: > > empty = !XFS_IFORK_Q(); > > > > default: > > empty = true; > > > > if ((is not btree) && (empty || nextents > 0)) > > return 0; > > > > --D > > > > > Brian > > > > > > > > Signed-off-by: Christoph Hellwig <hch@lst.de> > > > > > --- > > > > > fs/xfs/scrub/bmap.c | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/fs/xfs/scrub/bmap.c b/fs/xfs/scrub/bmap.c > > > > > index add8598eacd5d..283424d6d2bb6 100644 > > > > > --- a/fs/xfs/scrub/bmap.c > > > > > +++ b/fs/xfs/scrub/bmap.c > > > > > @@ -591,7 +591,7 @@ xchk_bmap_check_rmaps( > > > > > size = i_size_read(VFS_I(sc->ip)); > > > > > break; > > > > > case XFS_ATTR_FORK: > > > > > - size = XFS_IFORK_Q(sc->ip); > > > > > + size = XFS_IFORK_BOFF(sc->ip); > > > > > break; > > > > > default: > > > > > size = 0; > > > > > > > > > > > > > > > > > -- > > > > chandan > > > > > > > > > > > > > > > > > >
On Tue, May 12, 2020 at 12:16:15PM -0700, Darrick J. Wong wrote: > On Tue, May 12, 2020 at 12:14:10PM -0400, Brian Foster wrote: > > On Tue, May 12, 2020 at 08:38:54AM -0700, Darrick J. Wong wrote: > > > On Tue, May 12, 2020 at 11:31:32AM -0400, Brian Foster wrote: > > > > On Mon, May 11, 2020 at 05:10:04PM +0530, Chandan Babu R wrote: > > > > > On Sunday 10 May 2020 12:53:59 PM IST Christoph Hellwig wrote: > > > > > > XFS_IFORK_Q is to be used in boolean context, not for a size. This > > > > > > doesn't make a difference in practice as size is only checked for > > > > > > 0, but this keeps the logic sane. > > > > > > > > > > > > > > > > Wouldn't XFS_IFORK_ASIZE() be a better fit since it gives the space used by the > > > > > attr fork inside an inode's literal area? > > > > > > > > > > > > > I had the same thought. It's not clear to me what size is really > > > > supposed to be between the file size for a data fork and fork offset for > > > > the attr fork. I was also wondering if this should use > > > > XFS_IFORK_DSIZE(), but that won't be conditional based on population of > > > > the fork. At the same time, I don't think i_size != 0 necessarily > > > > correlates with the existence of blocks. The file could be completely > > > > sparse or could have any number of post-eof preallocated extents. > > > > > > TBH I should have made that variable "bool empty" or something. > > > > > > case XFS_DATA_FORK: > > > empty = i_size_read() == 0; > > > > > > > Even that is somewhat unclear because it's tied to i_size. What about > > size == 0 && <post-eof extents>? > > /me stumbles around trying to remember under what circumstances do we > actually scan all the rmaps to make sure there's a corresponding bmap. > I think we're trying to avoid doing it for every file in the filesystem > because it's very slow... > > Oh right -- we do that for btree format forks because there are a lot of > extent records and therefore a high(er) chance of something getting > lost; or > > We scan all the rmaps for files that have nonzero size but zero extents, > because the (forthcoming) inode fork repair will reset damaged forks > back to extents format with zero extents, and this is how we will > trigger the bmap repair to get the extents mapped back into the file. > Not sure I follow. Does this mean we're basically looking at sparse files as "suspect" because we'd expect them to have extents? Why are we looking at i_size here instead of fork size like for the attr fork, for example? Either way it sounds like we're getting into some twisty repair logic so perhaps this is beyond the scope of Christoph's patches.. Brian > --D > > > > > Brian > > > > > case XFS_ATTR_FORK: > > > empty = !XFS_IFORK_Q(); > > > > > > default: > > > empty = true; > > > > > > if ((is not btree) && (empty || nextents > 0)) > > > return 0; > > > > > > --D > > > > > > > Brian > > > > > > > > > > Signed-off-by: Christoph Hellwig <hch@lst.de> > > > > > > --- > > > > > > fs/xfs/scrub/bmap.c | 2 +- > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/fs/xfs/scrub/bmap.c b/fs/xfs/scrub/bmap.c > > > > > > index add8598eacd5d..283424d6d2bb6 100644 > > > > > > --- a/fs/xfs/scrub/bmap.c > > > > > > +++ b/fs/xfs/scrub/bmap.c > > > > > > @@ -591,7 +591,7 @@ xchk_bmap_check_rmaps( > > > > > > size = i_size_read(VFS_I(sc->ip)); > > > > > > break; > > > > > > case XFS_ATTR_FORK: > > > > > > - size = XFS_IFORK_Q(sc->ip); > > > > > > + size = XFS_IFORK_BOFF(sc->ip); > > > > > > break; > > > > > > default: > > > > > > size = 0; > > > > > > > > > > > > > > > > > > > > > -- > > > > > chandan > > > > > > > > > > > > > > > > > > > > > > > > >
On Wed, May 13, 2020 at 09:19:50AM -0400, Brian Foster wrote: > > We scan all the rmaps for files that have nonzero size but zero extents, > > because the (forthcoming) inode fork repair will reset damaged forks > > back to extents format with zero extents, and this is how we will > > trigger the bmap repair to get the extents mapped back into the file. > > > > Not sure I follow. Does this mean we're basically looking at sparse > files as "suspect" because we'd expect them to have extents? Why are we > looking at i_size here instead of fork size like for the attr fork, for > example? Either way it sounds like we're getting into some twisty repair > logic so perhaps this is beyond the scope of Christoph's patches.. The patch can be easily dropped from the series. I just noticed something fishy while looking over the code and though the fix would be easy enough..
diff --git a/fs/xfs/scrub/bmap.c b/fs/xfs/scrub/bmap.c index add8598eacd5d..283424d6d2bb6 100644 --- a/fs/xfs/scrub/bmap.c +++ b/fs/xfs/scrub/bmap.c @@ -591,7 +591,7 @@ xchk_bmap_check_rmaps( size = i_size_read(VFS_I(sc->ip)); break; case XFS_ATTR_FORK: - size = XFS_IFORK_Q(sc->ip); + size = XFS_IFORK_BOFF(sc->ip); break; default: size = 0;
XFS_IFORK_Q is to be used in boolean context, not for a size. This doesn't make a difference in practice as size is only checked for 0, but this keeps the logic sane. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/xfs/scrub/bmap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)