* [PATCH] xfs_repair: fix totally broken unit conversion in directory invalidation
@ 2019-12-18 4:24 Darrick J. Wong
2019-12-18 4:26 ` Darrick J. Wong
2019-12-24 8:38 ` Christoph Hellwig
0 siblings, 2 replies; 4+ messages in thread
From: Darrick J. Wong @ 2019-12-18 4:24 UTC (permalink / raw)
To: Eric Sandeen; +Cc: xfs
From: Darrick J. Wong <darrick.wong@oracle.com>
Your humble author forgot that xfs_dablk_t has the same units as
xfs_fileoff_t, and totally screwed up the directory buffer invalidation
loop in dir_binval. Not only is there an off-by-one error in the loop
conditional, but the unit conversions are wrong.
Fix all this stupidity by adding a for loop macro to take care of these
details for us so that everyone can iterate all logical directory blocks
(xfs_dir2_db_t) that start within a given bmbt record.
The pre-5.5 xfs_da_get_buf implementation mostly hides the off-by-one
error because dir_binval turns on "don't complain if no mapping" mode,
but on dirblocksize > fsblocksize filesystems the incorrect units can
cause us to miss invalidating some blocks, which can lead to other
buffer cache errors later.
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
libxfs/xfs_dir2.h | 10 ++++++++++
repair/phase6.c | 8 ++------
2 files changed, 12 insertions(+), 6 deletions(-)
diff --git a/libxfs/xfs_dir2.h b/libxfs/xfs_dir2.h
index f5424477..13221243 100644
--- a/libxfs/xfs_dir2.h
+++ b/libxfs/xfs_dir2.h
@@ -308,6 +308,16 @@ xfs_dir2_leaf_tail_p(struct xfs_da_geometry *geo, struct xfs_dir2_leaf *lp)
sizeof(struct xfs_dir2_leaf_tail));
}
+/*
+ * For a given dir/attr geometry and extent mapping record, walk every file
+ * offset block (xfs_dablk_t) in the mapping that corresponds to the start
+ * of a logical directory block (xfs_dir2_db_t).
+ */
+#define for_each_xfs_bmap_dabno(geo, irec, dabno) \
+ for ((dabno) = round_up((irec)->br_startoff, (geo)->fsbcount); \
+ (dabno) < (irec)->br_startoff + (irec)->br_blockcount; \
+ (dabno) += (geo)->fsbcount)
+
/*
* The Linux API doesn't pass down the total size of the buffer
* we read into down to the filesystem. With the filldir concept
diff --git a/repair/phase6.c b/repair/phase6.c
index 91d208a6..a4dd3188 100644
--- a/repair/phase6.c
+++ b/repair/phase6.c
@@ -1276,7 +1276,7 @@ dir_binval(
struct xfs_ifork *ifp;
struct xfs_da_geometry *geo;
struct xfs_buf *bp;
- xfs_dablk_t dabno, end_dabno;
+ xfs_dablk_t dabno;
int error = 0;
if (ip->i_d.di_format != XFS_DINODE_FMT_EXTENTS &&
@@ -1286,11 +1286,7 @@ dir_binval(
geo = tp->t_mountp->m_dir_geo;
ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
for_each_xfs_iext(ifp, &icur, &rec) {
- dabno = xfs_dir2_db_to_da(geo, rec.br_startoff +
- geo->fsbcount - 1);
- end_dabno = xfs_dir2_db_to_da(geo, rec.br_startoff +
- rec.br_blockcount);
- for (; dabno <= end_dabno; dabno += geo->fsbcount) {
+ for_each_xfs_bmap_dabno(geo, &rec, dabno) {
bp = NULL;
error = -libxfs_da_get_buf(tp, ip, dabno, -2, &bp,
whichfork);
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] xfs_repair: fix totally broken unit conversion in directory invalidation
2019-12-18 4:24 [PATCH] xfs_repair: fix totally broken unit conversion in directory invalidation Darrick J. Wong
@ 2019-12-18 4:26 ` Darrick J. Wong
2019-12-24 8:38 ` Christoph Hellwig
1 sibling, 0 replies; 4+ messages in thread
From: Darrick J. Wong @ 2019-12-18 4:26 UTC (permalink / raw)
To: Eric Sandeen; +Cc: xfs
On Tue, Dec 17, 2019 at 08:24:02PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> Your humble author forgot that xfs_dablk_t has the same units as
> xfs_fileoff_t, and totally screwed up the directory buffer invalidation
> loop in dir_binval. Not only is there an off-by-one error in the loop
> conditional, but the unit conversions are wrong.
>
> Fix all this stupidity by adding a for loop macro to take care of these
> details for us so that everyone can iterate all logical directory blocks
> (xfs_dir2_db_t) that start within a given bmbt record.
>
> The pre-5.5 xfs_da_get_buf implementation mostly hides the off-by-one
> error because dir_binval turns on "don't complain if no mapping" mode,
> but on dirblocksize > fsblocksize filesystems the incorrect units can
> cause us to miss invalidating some blocks, which can lead to other
> buffer cache errors later.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Fixes: f9c559f4e4fb4 ("xfs_repair: invalidate dirty dir buffers when we zap a directory")
--D
> ---
> libxfs/xfs_dir2.h | 10 ++++++++++
> repair/phase6.c | 8 ++------
> 2 files changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/libxfs/xfs_dir2.h b/libxfs/xfs_dir2.h
> index f5424477..13221243 100644
> --- a/libxfs/xfs_dir2.h
> +++ b/libxfs/xfs_dir2.h
> @@ -308,6 +308,16 @@ xfs_dir2_leaf_tail_p(struct xfs_da_geometry *geo, struct xfs_dir2_leaf *lp)
> sizeof(struct xfs_dir2_leaf_tail));
> }
>
> +/*
> + * For a given dir/attr geometry and extent mapping record, walk every file
> + * offset block (xfs_dablk_t) in the mapping that corresponds to the start
> + * of a logical directory block (xfs_dir2_db_t).
> + */
> +#define for_each_xfs_bmap_dabno(geo, irec, dabno) \
> + for ((dabno) = round_up((irec)->br_startoff, (geo)->fsbcount); \
> + (dabno) < (irec)->br_startoff + (irec)->br_blockcount; \
> + (dabno) += (geo)->fsbcount)
> +
> /*
> * The Linux API doesn't pass down the total size of the buffer
> * we read into down to the filesystem. With the filldir concept
> diff --git a/repair/phase6.c b/repair/phase6.c
> index 91d208a6..a4dd3188 100644
> --- a/repair/phase6.c
> +++ b/repair/phase6.c
> @@ -1276,7 +1276,7 @@ dir_binval(
> struct xfs_ifork *ifp;
> struct xfs_da_geometry *geo;
> struct xfs_buf *bp;
> - xfs_dablk_t dabno, end_dabno;
> + xfs_dablk_t dabno;
> int error = 0;
>
> if (ip->i_d.di_format != XFS_DINODE_FMT_EXTENTS &&
> @@ -1286,11 +1286,7 @@ dir_binval(
> geo = tp->t_mountp->m_dir_geo;
> ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
> for_each_xfs_iext(ifp, &icur, &rec) {
> - dabno = xfs_dir2_db_to_da(geo, rec.br_startoff +
> - geo->fsbcount - 1);
> - end_dabno = xfs_dir2_db_to_da(geo, rec.br_startoff +
> - rec.br_blockcount);
> - for (; dabno <= end_dabno; dabno += geo->fsbcount) {
> + for_each_xfs_bmap_dabno(geo, &rec, dabno) {
> bp = NULL;
> error = -libxfs_da_get_buf(tp, ip, dabno, -2, &bp,
> whichfork);
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] xfs_repair: fix totally broken unit conversion in directory invalidation
2019-12-18 4:24 [PATCH] xfs_repair: fix totally broken unit conversion in directory invalidation Darrick J. Wong
2019-12-18 4:26 ` Darrick J. Wong
@ 2019-12-24 8:38 ` Christoph Hellwig
2019-12-24 16:52 ` Darrick J. Wong
1 sibling, 1 reply; 4+ messages in thread
From: Christoph Hellwig @ 2019-12-24 8:38 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: Eric Sandeen, xfs
On Tue, Dec 17, 2019 at 08:24:02PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> Your humble author forgot that xfs_dablk_t has the same units as
> xfs_fileoff_t, and totally screwed up the directory buffer invalidation
> loop in dir_binval. Not only is there an off-by-one error in the loop
> conditional, but the unit conversions are wrong.
Can we kill off xfs_dablk_t? I found the concept very, very confusing
when touching the dir code.
> --- a/libxfs/xfs_dir2.h
> +++ b/libxfs/xfs_dir2.h
> @@ -308,6 +308,16 @@ xfs_dir2_leaf_tail_p(struct xfs_da_geometry *geo, struct xfs_dir2_leaf *lp)
> sizeof(struct xfs_dir2_leaf_tail));
> }
>
> +/*
> + * For a given dir/attr geometry and extent mapping record, walk every file
> + * offset block (xfs_dablk_t) in the mapping that corresponds to the start
> + * of a logical directory block (xfs_dir2_db_t).
> + */
> +#define for_each_xfs_bmap_dabno(geo, irec, dabno) \
> + for ((dabno) = round_up((irec)->br_startoff, (geo)->fsbcount); \
> + (dabno) < (irec)->br_startoff + (irec)->br_blockcount; \
> + (dabno) += (geo)->fsbcount)
I think not having the magic for macro would be cleaner..
> + xfs_dablk_t dabno;
> int error = 0;
>
> if (ip->i_d.di_format != XFS_DINODE_FMT_EXTENTS &&
> @@ -1286,11 +1286,7 @@ dir_binval(
> geo = tp->t_mountp->m_dir_geo;
> ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
> for_each_xfs_iext(ifp, &icur, &rec) {
> - dabno = xfs_dir2_db_to_da(geo, rec.br_startoff +
> - geo->fsbcount - 1);
> - end_dabno = xfs_dir2_db_to_da(geo, rec.br_startoff +
> - rec.br_blockcount);
> - for (; dabno <= end_dabno; dabno += geo->fsbcount) {
> + for_each_xfs_bmap_dabno(geo, &rec, dabno) {
> bp = NULL;
> error = -libxfs_da_get_buf(tp, ip, dabno, -2, &bp,
> whichfork);
But either way, the fix looks good.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] xfs_repair: fix totally broken unit conversion in directory invalidation
2019-12-24 8:38 ` Christoph Hellwig
@ 2019-12-24 16:52 ` Darrick J. Wong
0 siblings, 0 replies; 4+ messages in thread
From: Darrick J. Wong @ 2019-12-24 16:52 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Eric Sandeen, xfs
On Tue, Dec 24, 2019 at 12:38:43AM -0800, Christoph Hellwig wrote:
> On Tue, Dec 17, 2019 at 08:24:02PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> >
> > Your humble author forgot that xfs_dablk_t has the same units as
> > xfs_fileoff_t, and totally screwed up the directory buffer invalidation
> > loop in dir_binval. Not only is there an off-by-one error in the loop
> > conditional, but the unit conversions are wrong.
>
> Can we kill off xfs_dablk_t? I found the concept very, very confusing
> when touching the dir code.
I personally wouldn't mind if that happened (as a separate patch).
> > --- a/libxfs/xfs_dir2.h
> > +++ b/libxfs/xfs_dir2.h
> > @@ -308,6 +308,16 @@ xfs_dir2_leaf_tail_p(struct xfs_da_geometry *geo, struct xfs_dir2_leaf *lp)
> > sizeof(struct xfs_dir2_leaf_tail));
> > }
> >
> > +/*
> > + * For a given dir/attr geometry and extent mapping record, walk every file
> > + * offset block (xfs_dablk_t) in the mapping that corresponds to the start
> > + * of a logical directory block (xfs_dir2_db_t).
> > + */
> > +#define for_each_xfs_bmap_dabno(geo, irec, dabno) \
> > + for ((dabno) = round_up((irec)->br_startoff, (geo)->fsbcount); \
> > + (dabno) < (irec)->br_startoff + (irec)->br_blockcount; \
> > + (dabno) += (geo)->fsbcount)
>
> I think not having the magic for macro would be cleaner..
Eh, yeah, we don't really need it...
--D
>
> > + xfs_dablk_t dabno;
> > int error = 0;
> >
> > if (ip->i_d.di_format != XFS_DINODE_FMT_EXTENTS &&
> > @@ -1286,11 +1286,7 @@ dir_binval(
> > geo = tp->t_mountp->m_dir_geo;
> > ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
> > for_each_xfs_iext(ifp, &icur, &rec) {
> > - dabno = xfs_dir2_db_to_da(geo, rec.br_startoff +
> > - geo->fsbcount - 1);
> > - end_dabno = xfs_dir2_db_to_da(geo, rec.br_startoff +
> > - rec.br_blockcount);
> > - for (; dabno <= end_dabno; dabno += geo->fsbcount) {
> > + for_each_xfs_bmap_dabno(geo, &rec, dabno) {
> > bp = NULL;
> > error = -libxfs_da_get_buf(tp, ip, dabno, -2, &bp,
> > whichfork);
>
> But either way, the fix looks good.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-12-24 16:52 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-18 4:24 [PATCH] xfs_repair: fix totally broken unit conversion in directory invalidation Darrick J. Wong
2019-12-18 4:26 ` Darrick J. Wong
2019-12-24 8:38 ` Christoph Hellwig
2019-12-24 16:52 ` Darrick J. Wong
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.