Skip to content

Commit ad1858d

Browse files
Dave ChinnerBen Myers
authored andcommitted
xfs: rework remote attr CRCs
Note: this changes the on-disk remote attribute format. I assert that this is OK to do as CRCs are marked experimental and the first kernel it is included in has not yet reached release yet. Further, the userspace utilities are still evolving and so anyone using this stuff right now is a developer or tester using volatile filesystems for testing this feature. Hence changing the format right now to save longer term pain is the right thing to do. The fundamental change is to move from a header per extent in the attribute to a header per filesytem block in the attribute. This means there are more header blocks and the parsing of the attribute data is slightly more complex, but it has the advantage that we always know the size of the attribute on disk based on the length of the data it contains. This is where the header-per-extent method has problems. We don't know the size of the attribute on disk without first knowing how many extents are used to hold it. And we can't tell from a mapping lookup, either, because remote attributes can be allocated contiguously with other attribute blocks and so there is no obvious way of determining the actual size of the atribute on disk short of walking and mapping buffers. The problem with this approach is that if we map a buffer incorrectly (e.g. we make the last buffer for the attribute data too long), we then get buffer cache lookup failure when we map it correctly. i.e. we get a size mismatch on lookup. This is not necessarily fatal, but it's a cache coherency problem that can lead to returning the wrong data to userspace or writing the wrong data to disk. And debug kernels will assert fail if this occurs. I found lots of niggly little problems trying to fix this issue on a 4k block size filesystem, finally getting it to pass with lots of fixes. The thing is, 1024 byte filesystems still failed, and it was getting really complex handling all the corner cases that were showing up. And there were clearly more that I hadn't found yet. It is complex, fragile code, and if we don't fix it now, it will be complex, fragile code forever more. Hence the simple fix is to add a header to each filesystem block. This gives us the same relationship between the attribute data length and the number of blocks on disk as we have without CRCs - it's a linear mapping and doesn't require us to guess anything. It is simple to implement, too - the remote block count calculated at lookup time can be used by the remote attribute set/get/remove code without modification for both CRC and non-CRC filesystems. The world becomes sane again. Because the copy-in and copy-out now need to iterate over each filesystem block, I moved them into helper functions so we separate the block mapping and buffer manupulations from the attribute data and CRC header manipulations. The code becomes much clearer as a result, and it is a lot easier to understand and debug. It also appears to be much more robust - once it worked on 4k block size filesystems, it has worked without failure on 1k block size filesystems, too. Signed-off-by: Dave Chinner <[email protected]> Reviewed-by: Ben Myers <[email protected]> Signed-off-by: Ben Myers <[email protected]>
1 parent d4c712b commit ad1858d

File tree

4 files changed

+247
-158
lines changed

4 files changed

+247
-158
lines changed

fs/xfs/xfs_attr_leaf.c

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1412,7 +1412,7 @@ xfs_attr3_leaf_add_work(
14121412
name_rmt->valuelen = 0;
14131413
name_rmt->valueblk = 0;
14141414
args->rmtblkno = 1;
1415-
args->rmtblkcnt = XFS_B_TO_FSB(mp, args->valuelen);
1415+
args->rmtblkcnt = xfs_attr3_rmt_blocks(mp, args->valuelen);
14161416
}
14171417
xfs_trans_log_buf(args->trans, bp,
14181418
XFS_DA_LOGRANGE(leaf, xfs_attr3_leaf_name(leaf, args->index),
@@ -2354,8 +2354,9 @@ xfs_attr3_leaf_lookup_int(
23542354
args->index = probe;
23552355
args->valuelen = be32_to_cpu(name_rmt->valuelen);
23562356
args->rmtblkno = be32_to_cpu(name_rmt->valueblk);
2357-
args->rmtblkcnt = XFS_B_TO_FSB(args->dp->i_mount,
2358-
args->valuelen);
2357+
args->rmtblkcnt = xfs_attr3_rmt_blocks(
2358+
args->dp->i_mount,
2359+
args->valuelen);
23592360
return XFS_ERROR(EEXIST);
23602361
}
23612362
}
@@ -2406,7 +2407,8 @@ xfs_attr3_leaf_getvalue(
24062407
ASSERT(memcmp(args->name, name_rmt->name, args->namelen) == 0);
24072408
valuelen = be32_to_cpu(name_rmt->valuelen);
24082409
args->rmtblkno = be32_to_cpu(name_rmt->valueblk);
2409-
args->rmtblkcnt = XFS_B_TO_FSB(args->dp->i_mount, valuelen);
2410+
args->rmtblkcnt = xfs_attr3_rmt_blocks(args->dp->i_mount,
2411+
valuelen);
24102412
if (args->flags & ATTR_KERNOVAL) {
24112413
args->valuelen = valuelen;
24122414
return 0;
@@ -2732,7 +2734,8 @@ xfs_attr3_leaf_list_int(
27322734
args.valuelen = valuelen;
27332735
args.value = kmem_alloc(valuelen, KM_SLEEP | KM_NOFS);
27342736
args.rmtblkno = be32_to_cpu(name_rmt->valueblk);
2735-
args.rmtblkcnt = XFS_B_TO_FSB(args.dp->i_mount, valuelen);
2737+
args.rmtblkcnt = xfs_attr3_rmt_blocks(
2738+
args.dp->i_mount, valuelen);
27362739
retval = xfs_attr_rmtval_get(&args);
27372740
if (retval)
27382741
return retval;

0 commit comments

Comments
 (0)