Re: [PATCH] (and bad attr2 bug) - pack xfs_sb_t for 64-bit arches

Russell Cattelan
2006-11-22T01:23:31Z

[-- Attachment #1.1: Type: text/plain, Size: 1769 bytes --]

On Mon, 2006-11-20 at 22:02 -0600, Eric Sandeen wrote:
> Eric Sandeen wrote:
>
> > Eric Sandeen wrote:
> >
> >> ugh. it's broken on x86 too, so it's not just the alignment/padding,
> >>
> >> although that should be fixed for cross-arch mounts.
> >>
> >> -Eric
> >>
> >>
> > here's a testcase to corrupt it FWIW.
> >
> >
> Ok, with expert collaboration from Russell, Barry, Tim,
> Nathan, David, et al, how about this:
>
> For btree dirs, we need a different calculation for the space
> used in di_u, to set the minimum threshold for the fork offset...
>
> This fixes my testcase, but as Tim points out -now- we need to compact
> the btree ptrs, if we return (and use) an offset < current forkoff...
>
> whee....
>
> -Eric
>
It turns out this only fixes one of the problems it is still quite easy
to corrupt indoes with attr2.

The following patch is a short term fix that address the problem of
forkoff
moving without re-factoring the root inode btree root block.

Once the inode has be flipped to BTREE for the data space the forkoff is
fixed
to the that size, currently due to the way attr1 worked (fixed size
forkoff) the code is not handling the size to the root btree node due to
size changes in the attr portion of the inode.

The optimal solution is to adjust the data portion of the inode root
btree block down if space exists.

One easy fix that was resulting all attr add being pushed out of line is
added
the header size to the initial split of the inode, at least the first
attr add
should go inline now. Which should be a win the big attr user right now
SElinux.

Including the 2 test script that have been used.

--
Russell Cattelan <cattelan@thebarn.com>

[-- Attachment #1.2: attr2_patch --]
[-- Type: text/x-patch, Size: 5208 bytes --]

Index: work_gfs/fs/xfs/xfs_attr.c
===================================================================
--- work_gfs.orig/fs/xfs/xfs_attr.c 2006-11-21 18:38:27.572949303 -0600
+++ work_gfs/fs/xfs/xfs_attr.c 2006-11-21 18:44:51.666033422 -0600
@@ -210,8 +210,20 @@ xfs_attr_set_int(xfs_inode_t *dp, const
* (inode must not be locked when we call this routine)
*/
if (XFS_IFORK_Q(dp) == 0) {
- if ((error = xfs_bmap_add_attrfork(dp, size, rsvd)))
- return(error);
+ if ((dp->i_d.di_aformat == XFS_DINODE_FMT_LOCAL) ||
+ ((dp->i_d.di_aformat == XFS_DINODE_FMT_EXTENTS) &&
+ (dp->i_d.di_anextents == 0))) {
+ /* xfs_bmap_add_attrfork will set the forkoffset based on
+ * the size needed, the local attr case needs the size
+ * attr plus the size of the hdr, if the size of
+ * header is not accounted for initially the forkoffset
+ * won't allow enough space, the actually attr add will
+ * then be forced out out line to extents
+ */
+ size += sizeof(xfs_attr_sf_hdr_t);
+ if ((error = xfs_bmap_add_attrfork(dp, size, rsvd)))
+ return(error);
+ }
}

/*
Index: work_gfs/fs/xfs/xfs_attr_leaf.c
===================================================================
--- work_gfs.orig/fs/xfs/xfs_attr_leaf.c 2006-11-21 18:38:27.572949303 -0600
+++ work_gfs/fs/xfs/xfs_attr_leaf.c 2006-11-21 18:45:16.094360919 -0600
@@ -148,40 +148,76 @@ int
xfs_attr_shortform_bytesfit(xfs_inode_t *dp, int bytes)
{
int offset;
- int minforkoff; /* lower limit on valid forkoff locations */
- int maxforkoff; /* upper limit on valid forkoff locations */
+ int minforkoff = 0; /* lower limit on valid forkoff locations */
+ int maxforkoff = 0; /* upper limit on valid forkoff locations */
xfs_mount_t *mp = dp->i_mount;
+ int result = 0;
+ int dsize = 0;

offset = (XFS_LITINO(mp) - bytes) >> 3; /* rounded down */

switch (dp->i_d.di_format) {
case XFS_DINODE_FMT_DEV:
minforkoff = roundup(sizeof(xfs_dev_t), 8) >> 3;
- return (offset >= minforkoff) ? minforkoff : 0;
+ result = (offset >= minforkoff) ? minforkoff : 0;
+ goto result;
case XFS_DINODE_FMT_UUID:
minforkoff = roundup(sizeof(uuid_t), 8) >> 3;
- return (offset >= minforkoff) ? minforkoff : 0;
+ result = (offset >= minforkoff) ? minforkoff : 0;
+ goto result;
}

if (!(mp->m_flags & XFS_MOUNT_ATTR2)) {
- if (bytes <= XFS_IFORK_ASIZE(dp))
- return mp->m_attroffset >> 3;
- return 0;
+ if (bytes <= XFS_IFORK_ASIZE(dp)) {
+ result = mp->m_attroffset >> 3;
+ goto result;
+ }
+ result = 0;
+ goto result;
}

/* data fork btree root can have at least this many key/ptr pairs */
- minforkoff = MAX(dp->i_df.if_bytes, XFS_BMDR_SPACE_CALC(MINDBTPTRS));
+ switch (dp->i_d.di_format) {
+ case XFS_DINODE_FMT_LOCAL:
+ case XFS_DINODE_FMT_EXTENTS:
+ dsize = dp->i_df.if_bytes;
+ break;
+ case XFS_DINODE_FMT_BTREE:
+ if (dp->i_d.di_forkoff)
+ dsize = dp->i_d.di_forkoff << 3;
+ else
+ dsize = XFS_BMDR_SPACE_CALC(
+ XFS_BMAP_BROOT_NUMRECS(dp->i_df.if_broot));
+ break;
+ default:
+ break;
+ }
+
+ minforkoff = MAX(dsize, XFS_BMDR_SPACE_CALC(MINDBTPTRS));
minforkoff = roundup(minforkoff, 8) >> 3;

/* attr fork btree root can have at least this many key/ptr pairs */
maxforkoff = XFS_LITINO(mp) - XFS_BMDR_SPACE_CALC(MINABTPTRS);
maxforkoff = maxforkoff >> 3; /* rounded down */

- if (offset >= minforkoff && offset < maxforkoff)
- return offset;
- if (offset >= maxforkoff)
- return maxforkoff;
- return 0;
+ if (offset >= minforkoff && offset < maxforkoff) {
+ result = offset;
+ }
+
+ if (offset >= maxforkoff) {
+ result = maxforkoff;
+ }
+
+ /* the case of btree we don't want to move the forkoff
+ * since that would require a rebalance of the btree
+ * which is currently not implemented for attr2
+ */
+
+ if (dp->i_d.di_format == XFS_DINODE_FMT_BTREE && result) {
+ result = dsize >> 3;
+ }
+result:
+ return result;
}

/*
Index: work_gfs/fs/xfs/xfs_bmap.c
===================================================================
--- work_gfs.orig/fs/xfs/xfs_bmap.c 2006-11-21 18:38:27.572949303 -0600
+++ work_gfs/fs/xfs/xfs_bmap.c 2006-11-21 18:45:38.446660445 -0600
@@ -3632,7 +3632,26 @@ xfs_bmap_local_to_extents(
flags |= XFS_ILOG_FEXT(whichfork);
} else {
ASSERT(XFS_IFORK_NEXTENTS(ip, whichfork) == 0);
- xfs_bmap_forkoff_reset(ip->i_mount, ip, whichfork);
+
+ /* Changing the forkoff requires a rebalance
+ of the data btree.
+ Once forkoff is set leave it fixed while
+ data format is stored in a btree.
+
+ Granted the inode btree block in initially
+ setup with the max avalible DSIZE space so
+ might be a slight waste of space to not
+ rebalance.
+ But since the attr1 code did not have a
+ variable forkoff as attr2 now has the
+ detection points and the reblanace code
+ it not in place.
+ Take the easy way out for now
+ */
+
+ if (ip->i_d.di_format != XFS_DINODE_FMT_BTREE)
+ xfs_bmap_forkoff_reset(ip->i_mount, ip, whichfork);
+
}
ifp->if_flags &= ~XFS_IFINLINE;
ifp->if_flags |= XFS_IFEXTENTS;

[-- Attachment #1.3: r2.sh --]
[-- Type: application/x-shellscript, Size: 569 bytes --]

[-- Attachment #1.4: e2 --]
[-- Type: application/x-shellscript, Size: 713 bytes --]

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]