xfsprogs: CRC support
PATCH 45/48 xfs_repair: fix btree block magic number mapping
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
The magic numbers for generic btree blocks were modified some time ago (before the kernel code was committed) but the xfs_repair mapping code was not updated to match. It's no longer a simple mapping, so just make the code a dense array and use the magic number as the search key.
--- db/btblock.c | 37 +++++++++++++++++++++++++------------ 1 file changed, 25 insertions(+), 12 deletions(-)
db/btblock.c +25 -12
@@ -26,65 +26,66 @@#include "bit.h" #include "init.h" -/* * Definition of the possible btree block layouts. */ struct xfs_db_btree { +uint32_t magic;size_t block_len;size_t key_len;size_t rec_len;size_t ptr_len; } btrees[] = { - [/*0x424d415*/ 0 ] = {/* BMAP */ + {XFS_BMAP_MAGIC ,XFS_BTREE_LBLOCK_LEN ,sizeof (xfs_bmbt_key_t),sizeof (xfs_bmbt_rec_t),sizeof (__be64), }, - [/*0x4142544*/ 2 ] = {/* ABTB */ + {XFS_ABTB_MAGIC ,XFS_BTREE_SBLOCK_LEN ,sizeof (xfs_alloc_key_t),sizeof (xfs_alloc_rec_t),sizeof (__be32), }, - [/*0x4142544*/ 3 ] = {/* ABTC */ + {XFS_ABTC_MAGIC ,XFS_BTREE_SBLOCK_LEN ,sizeof (xfs_alloc_key_t),sizeof (xfs_alloc_rec_t),sizeof (__be32), }, - [/*0x4941425*/ 4 ] = {/* IABT */ + {XFS_IBT_MAGIC ,XFS_BTREE_SBLOCK_LEN ,sizeof (xfs_inobt_key_t),sizeof (xfs_inobt_rec_t),sizeof (__be32), }, - [/*0x424d415*/ 8 ] = {/* BMAP_CRC */ + {XFS_BMAP_CRC_MAGIC ,XFS_BTREE_LBLOCK_CRC_LEN ,sizeof (xfs_bmbt_key_t),sizeof (xfs_bmbt_rec_t),sizeof (__be64), }, - [/*0x4142544*/ 0xa ] = {/* ABTB_CRC */ + {XFS_ABTB_CRC_MAGIC ,XFS_BTREE_SBLOCK_CRC_LEN ,sizeof (xfs_alloc_key_t),sizeof (xfs_alloc_rec_t),sizeof (__be32), }, - [/*0x414254*/ 0xb ] = {/* ABTC_CRC */ + {XFS_ABTC_CRC_MAGIC ,XFS_BTREE_SBLOCK_CRC_LEN ,sizeof (xfs_alloc_key_t),sizeof (xfs_alloc_rec_t),sizeof (__be32), }, - [/*0x4941425*/ 0xc ] = {/* IABT_CRC */ + {XFS_IBT_CRC_MAGIC ,XFS_BTREE_SBLOCK_CRC_LEN ,sizeof (xfs_inobt_key_t),sizeof (xfs_inobt_rec_t),sizeof (__be32), }, - + {0 , + }, };/*
@@ -93,8 +94,20 @@ struct xfs_db_btree {* We use the least significant bit of the magic number as index into * the array of block defintions. */ -#define block_to_bt (bb) \ - (&btrees[be32_to_cpu((bb)->bb_magic) & 0xf]) +static struct xfs_db_btree * +block_to_bt ( +struct xfs_btree_block *bb) +{ +struct xfs_db_btree *btp = &btrees[0 ]; + +do { +if (be32_to_cpu ((bb)->bb_magic) == btp->magic) +return btp; + btp++; + }while (btp->magic !=0 ); + +return NULL ; +}/* calculate max records. Only for non-leaves. */ static int
Signature · 6 lines
1.7.10.4 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 45/48] xfs_repair: fix btree block magic number mapping
Quoted 9 lines · Dave Chinner
From: Dave Chinner <[email protected]>
The magic numbers for generic btree blocks were modified some time ago (before the kernel code was committed) but the
xfs_repairmapping code was not updated to match. It's no longer a simple mapping, so just make the code a dense array and use the magic number as the search key.Signed-off-by Dave Chinner <[email protected]>
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 46/48 libxfs: fix dir3 freespace block corruption
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
When the directory freespace index grows to a second block (2017 4k data blocks in the directory), the initialisation of the second new block header goes wrong. The write verifier fires a corruption error indicating that the block number in the header is zero. This was being tripped by xfs/110.
The problem is that the initialisation of the new block is done just fine in xfs_dir3_free_get_buf(), but the caller then users a dirv2 structure to zero on-disk header fields that xfs_dir3_free_get_buf() has already zeroed. These lined up with the block number in the dir v3 header format.
While looking at this, I noticed that the struct xfs_dir3_free_hdr() had 4 bytes of padding in it that wasn't defined as padding or being zeroed by the initialisation. Add a pad field declaration and fully zero the on disk and in-core headers in xfs_dir3_free_get_buf() so that this is never an issue in the future. Note that this doesn't change the on-disk layout, just makes the 32 bits of padding in the layout explicit.
--- include/xfs_dir2_format.h | 1 + libxfs/xfs_dir2_node.c | 13 ++++++------- 2 files changed, 7 insertions(+), 7 deletions(-)
include/xfs_dir2_format.h +1 -0
@@ -712,6 +712,7 @@ struct xfs_dir3_free_hdr {__be32 firstdb;/* db of first entry */ __be32 nvalid;/* count of valid entries */ __be32 nused;/* count of used entries */ +__be32 pad;/* 64 bit alignment. */ };struct xfs_dir3_free {
libxfs/xfs_dir2_node.c +6 -7
@@ -246,19 +246,20 @@ xfs_dir3_free_get_buf(* Initialize the new block to be empty, and remember * its first slot as our empty slot. */ - hdr.magic =XFS_DIR2_FREE_MAGIC ; - hdr.firstdb =0 ; - hdr.nused =0 ; - hdr.nvalid =0 ; +memset (bp->b_addr,0 ,sizeof (struct xfs_dir3_free_hdr )); +memset (&hdr,0 ,sizeof (hdr)); +if (xfs_sb_version_hascrc (&mp->m_sb)) {struct xfs_dir3_free_hdr *hdr3 = bp->b_addr; hdr.magic =XFS_DIR3_FREE_MAGIC ; + hdr3->hdr.blkno =cpu_to_be64 (bp->b_bn); hdr3->hdr.owner =cpu_to_be64 (dp->i_ino);uuid_copy (&hdr3->hdr.uuid, &mp->m_sb.sb_uuid); - } + }else + hdr.magic =XFS_DIR2_FREE_MAGIC ;xfs_dir3_free_hdr_to_disk (bp->b_addr, &hdr); *bpp = bp;return 0 ;
@@ -1906,8 +1907,6 @@ xfs_dir2_node_addname_int(*/ freehdr.firstdb = (fbno -XFS_DIR2_FREE_FIRSTDB (mp)) *xfs_dir3_free_max_bests (mp); - free->hdr.nvalid =0 ; - free->hdr.nused =0 ; }else { free = fbp->b_addr; bests =xfs_dir3_free_bests_p (mp, free);
Signature · 6 lines
1.7.10.4 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 46/48] libxfs: fix dir3 freespace block corruption
Quoted 22 lines · Dave Chinner
From: Dave Chinner <[email protected]>
When the directory freespace index grows to a second block (2017 4k data blocks in the directory), the initialisation of the second new block header goes wrong. The write verifier fires a corruption error indicating that the block number in the header is zero. This was being tripped by xfs/110.
The problem is that the initialisation of the new block is done just fine in
xfs_dir3_free_get_buf(), but the caller then users a dirv2 structure to zero on-disk header fields thatxfs_dir3_free_get_buf()has already zeroed. These lined up with the block number in the dir v3 header format.While looking at this, I noticed that the struct
xfs_dir3_free_hdr()had 4 bytes of padding in it that wasn't defined as padding or being zeroed by the initialisation. Add a pad field declaration and fully zero the on disk and in-core headers inxfs_dir3_free_get_buf()so that this is never an issue in the future. Note that this doesn't change the on-disk layout, just makes the 32 bits of padding in the layout explicit.
Quoted 1 line
Signed-off-by Dave Chinner <[email protected]>
Goes with commit 5170711df79b28
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 47/48 xfs_repair: support CRC enabled remote symlinks
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
Add support for verifying the contents of remote symlinks with CRCs. Factor the remote symlink checking code out of the symlink function so that it is clear what it is checking. This also reduces the indentation and makes the code clearer.
Then add support for the CRC format by modelling the checking function directly on the code that is used in the kernel for reading and checking both remote symlink formats.
--- libxfs/xfs_symlink.c | 11 +---- repair/dinode.c | 132 ++++++++++++++++++++++++++++++++------------------ 2 files changed, 88 insertions(+), 55 deletions(-)
libxfs/xfs_symlink.c +2 -9
@@ -14,16 +14,9 @@ xfs_symlink_blocks(struct xfs_mount *mp, int pathlen) { - int fsblocks = 0; -int len = pathlen; + int buflen = XFS_SYMLINK_BUF_SPACE(mp, mp->m_sb.sb_blocksize); -do { - fsblocks++; - len -=XFS_SYMLINK_BUF_SPACE (mp, mp->m_sb.sb_blocksize); - }while (len >0 ); - -ASSERT (fsblocks <=XFS_SYMLINK_MAPS ); -return fsblocks; +return (pathlen + buflen -1 ) / buflen; }/*
repair/dinode.c +86 -46
152 lines not on this page · raw message
Signature · 6 lines
1.7.10.4 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 47/48] xfs_repair: support CRC enabled remote symlinks
Quoted 7 lines · Dave Chinner
From: Dave Chinner <[email protected]>
Add support for verifying the contents of remote symlinks with CRCs. Factor the remote symlink checking code out of the symlink function so that it is clear what it is checking. This also reduces the indentation and makes the code clearer.
Quoted 5 lines
Then add support for the CRC format by modelling the checking function directly on the code that is used in the kernel for reading and checking both remote symlink formats.
Signed-off-by Dave Chinner <[email protected]>
This goes with commit 321a95839e65.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 48/48 xfsprogs: Document XFs specific mount options in xfs(5)
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
Rather than reference mount(8) to see xfs specific mount options, document them directly in the xfs(5) man page in this package. That way it is easy to update XFS mount options when the change.
--- man/man5/xfs.5 | 197 +++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 194 insertions(+), 3 deletions(-)
man/man5/xfs.5 +194 -3
@@ -98,9 +98,200 @@ and by-handle (see .BR open_by_handle (3)) interfaces. .SH MOUNT OPTIONS -Refer to the -.BR mount (8) -manual entry for descriptions of the individual XFS mount options. +.TP +.BI allocsize= size +Sets the buffered I/O end-of-file preallocation size when +doing delayed allocation writeout (default size is 64KiB). +Valid values for this option are page size (typically 4KiB) +through to 1GiB, inclusive, in power-of-2 increments. +.TP +.BR attr2 | noattr2 +The options enable/disable (default is enabled) an "opportunistic" +improvement to be made in the way inline extended attributes are +stored on-disk. +When the new form is used for the first time (by setting or +removing extended attributes) the on-disk superblock feature +bit field will be updated to reflect this format being in use. +.TP +.B barrier | nobarrier +Enables/disables the use of block layer write barriers for writes into +the journal and for data integrity operations. +This allows for drive level write caching to be enabled, for devices that +support write barriers. +The default behaviour is to have barriers enabled. +.TP +.BR ikeep | noikeep +When inode clusters are emptied of inodes, keep them around +on the disk (ikeep) - this is the traditional XFS behaviour +and is still the default for now. Using the noikeep option, +inode clusters are returned to the free space pool. +.TP +.B inode64 +Indicates that XFS is allowed to create inodes at any location +in the filesystem, including those which will result in inode +numbers occupying more than 32 bits of significance. This is +provided for backwards compatibility, but causes problems for +backup applications that cannot handle large inode numbers. +.TP +.BR largeio | nolargeio +If +.B nolargeio +is specified, the optimal I/O reported in +st_blksize by +.BR stat (2) +will be as small as possible to allow user +applications to avoid inefficient read/modify/write I/O. +If +.B largeio +is specified, a filesystem that has a +.B swidth +specified +will return the +.B swidth +value (in bytes) in st_blksize. If the +filesystem does not have a +.B swidth +specified but does specify +an +.B allocsize +then +.B allocsize +(in bytes) will be returned +instead. +If neither of these two options are specified, then filesystem +will behave as if +.B nolargeio +was specified. +.TP +.BI logbufs= value +Set the number of in-memory log buffers. Valid numbers range +from 2-8 inclusive. +The default value is 8 buffers for any recent kernel. +.TP +.BI logbsize= value +Set the size of each in-memory log buffer. +Size may be specified in bytes, or in kilobytes with a "k" suffix. +Valid sizes for version 1 and version 2 logs are 16384 (16k) and +32768 (32k). Valid sizes for version 2 logs also include +65536 (64k), 131072 (128k) and 262144 (256k). +The default value for any recent kernel is 32768. +.TP +\fBlogdev=\fP\fIdevice\fP and \fBrtdev=\fP\fIdevice\fP +Use an external log (metadata journal) and/or real-time device. +An XFS filesystem has up to three parts: a data section, a log section, +and a real-time section. +The real-time section is optional, and the log section can be separate +from the data section or contained within it. +Refer to +.BR xfs (5). +.TP +.BI mtpt= mountpoint +Use with the +.B dmapi +option. The value specified here will be +included in the DMAPI mount event, and should be the path of +the actual mountpoint that is used. +.TP +.B noalign +Data allocations will not be aligned at stripe unit boundaries. +.TP +.B noatime +Access timestamps are not updated when a file is read. +.TP +.B norecovery +The filesystem will be mounted without running log recovery. +If the filesystem was not cleanly unmounted, it is likely to +be inconsistent when mounted in +.B norecovery +mode. +Some files or directories may not be accessible because of this. +Filesystems mounted +.B norecovery +must be mounted read-only or the mount will fail. +.TP +.B nouuid +Don't check for double mounted filesystems using the filesystem uuid. +This is useful to mount LVM snapshot volumes. +.TP +.B osyncisosync +Make O_SYNC writes implement true O_SYNC. WITHOUT this option, +Linux XFS behaves as if an +.B osyncisdsync +option is used, +which will make writes to files opened with the O_SYNC flag set +behave as if the O_DSYNC flag had been used instead. +This can result in better performance without compromising +data safety. +However if this option is not in effect, timestamp updates from +O_SYNC writes can be lost if the system crashes. +If timestamp updates are critical, use the +.B osyncisosync +option. +.TP +.BR uquota | usrquota | uqnoenforce | quota +User disk quota accounting enabled, and limits (optionally) +enforced. Refer to +.BR xfs_quota (8) +for further details. +.TP +.BR gquota | grpquota | gqnoenforce +Group disk quota accounting enabled and limits (optionally) +enforced. Refer to +.BR xfs_quota (8) +for further details. +.TP +.BR pquota | prjquota | pqnoenforce +Project disk quota accounting enabled and limits (optionally) +enforced. Refer to +.BR xfs_quota (8) +for further details. +.TP +\fBsunit=\fP\fIvalue\fP and \fBswidth=\fP\fIvalue\fP +Used to specify the stripe unit and width for a RAID device or a stripe +volume. +.I value +must be specified in 512-byte block units. +If this option is not specified and the filesystem was made on a stripe +volume or the stripe width or unit were specified for the RAID device at +mkfs time, then the mount system call will restore the value from the +superblock. +For filesystems that are made directly on RAID devices, these options can be +used to override the information in the superblock if the underlying disk +layout changes after the filesystem has been created. +The +.B swidth +option is required if the +.B sunit +option has been specified, +and must be a multiple of the +.B sunit +value. +.TP +.B swalloc +Data allocations will be rounded up to stripe width boundaries +when the current end of file is being extended and the file +size is larger than the stripe width size. +.TP +.B dmapi +Enable the DMAPI (Data Management API) event callouts. +Use with the +.B mtpt +option. +.TP +.BR grpid | bsdgroups " and " nogrpid | sysvgroups +These options define what group ID a newly created file gets. +When grpid is set, it takes the group ID of the directory in +which it is created; otherwise (the default) it takes the fsgid +of the current process, unless the directory has the setgid bit +set, in which case it takes the gid from the parent directory, +and also gets the setgid bit set if it is a directory itself. +.TP +.BI ihashsize= value +Sets the number of hash buckets available for hashing the +in-memory inodes of the specified mount point. If a value +of zero is used, the value selected by the default algorithm +will be displayed in +.IR /proc/mounts . .SH SEE ALSO .BR xfsctl (3), .BR mount (8),
Signature · 6 lines
1.7.10.4 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 48/48] xfsprogs: Document XFs specific mount options in xfs(5)
Quoted 7 lines · Dave Chinner
From: Dave Chinner <[email protected]>
Rather than reference mount(8) to see xfs specific mount options, document them directly in the xfs(5) man page in this package. That way it is easy to update XFS mount options when the change.
Signed-off-by Dave Chinner <[email protected]>
I didn't mean to include this patch in the series - it's not completely up to date yet....
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Quoted 20 lines · Dave Chinner
Hi folks,
This is the latest update of the series of patches tht introduces CRC support into xfsprogs. Of note, for CRC enabled filesystems;
- write support for xfs-db is disabled
- obfuscation for metadump is disabled
-xfs_checkdoes nothing ("always succeed") so that xfstests can run without needing this
- all structures shoul dbe supported for printing inxfs_db
-xfs_repairshould be able to fully validate the structure of a CRC enabled filesystem.
-xfs_repairstill ignores CRC validation errors when reading metadata
- mkfs.xfs enforces limitations on the format of CRC enabled filesystems (inode size, attr format, projid32bit, etc).
- whenever a v5 superblock is parsed on read by any utility, it outputs a wanring about it being an experimental format.
Quoted 1 line
Bug reports, patches, comments, reviews, etc all welcome.
I've just realised that I haven't ported any of the recent kernel fixes across to this patch set, so there will be another few patches needed for those as well...
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Hey,
Quoted 21 lines · Dave Chinner
Quoted 20 lines · Dave Chinner
Hi folks,
This is the latest update of the series of patches tht introduces CRC support into xfsprogs. Of note, for CRC enabled filesystems;
- write support for xfs-db is disabled
- obfuscation for metadump is disabled
-xfs_checkdoes nothing ("always succeed") so that xfstests can run without needing this
- all structures shoul dbe supported for printing inxfs_db
-xfs_repairshould be able to fully validate the structure of a CRC enabled filesystem.
-xfs_repairstill ignores CRC validation errors when reading metadata
- mkfs.xfs enforces limitations on the format of CRC enabled filesystems (inode size, attr format, projid32bit, etc).
- whenever a v5 superblock is parsed on read by any utility, it outputs a wanring about it being an experimental format.
Quoted 5 lines
Quoted 1 line
Bug reports, patches, comments, reviews, etc all welcome.
I've just realised that I haven't ported any of the recent kernel fixes across to this patch set, so there will be another few patches needed for those as well...
These two series are applied to the crc-dev branch atgit://oss.sgi.com/xfs/cmds/xfsprogs.git
Regards,
Ben
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
This is helpful. Thanks.
Chandra
Quoted 33 lines · Ben Myers
Hey,
Quoted 21 lines · Dave Chinner
Quoted 20 lines · Dave Chinner
Hi folks,
This is the latest update of the series of patches tht introduces CRC support into xfsprogs. Of note, for CRC enabled filesystems;
- write support for xfs-db is disabled
- obfuscation for metadump is disabled
-xfs_checkdoes nothing ("always succeed") so that xfstests can run without needing this
- all structures shoul dbe supported for printing inxfs_db
-xfs_repairshould be able to fully validate the structure of a CRC enabled filesystem.
-xfs_repairstill ignores CRC validation errors when reading metadata
- mkfs.xfs enforces limitations on the format of CRC enabled filesystems (inode size, attr format, projid32bit, etc).
- whenever a v5 superblock is parsed on read by any utility, it outputs a wanring about it being an experimental format.Quoted 5 lines
Quoted 1 line
Bug reports, patches, comments, reviews, etc all welcome.
I've just realised that I haven't ported any of the recent kernel fixes across to this patch set, so there will be another few patches needed for those as well...
These two series are applied to the crc-dev branch at
git://oss.sgi.com/xfs/cmds/xfsprogs.git
Quoted 8 lines
Regards,
Ben_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 24 lines · Ben Myers
Hey,
Quoted 21 lines · Dave Chinner
Quoted 20 lines · Dave Chinner
Hi folks,
This is the latest update of the series of patches tht introduces CRC support into xfsprogs. Of note, for CRC enabled filesystems;
- write support for xfs-db is disabled
- obfuscation for metadump is disabled
-xfs_checkdoes nothing ("always succeed") so that xfstests can run without needing this
- all structures shoul dbe supported for printing inxfs_db
-xfs_repairshould be able to fully validate the structure of a CRC enabled filesystem.
-xfs_repairstill ignores CRC validation errors when reading metadata
- mkfs.xfs enforces limitations on the format of CRC enabled filesystems (inode size, attr format, projid32bit, etc).
- whenever a v5 superblock is parsed on read by any utility, it outputs a wanring about it being an experimental format.
Quoted 8 lines
Quoted 5 lines
Quoted 1 line
Bug reports, patches, comments, reviews, etc all welcome.
I've just realised that I haven't ported any of the recent kernel fixes across to this patch set, so there will be another few patches needed for those as well...
These two series are applied to the crc-dev branch at
git://oss.sgi.com/xfs/cmds/xfsprogs.git
Hi Ben,
Thanks for doing this. This patch, however:
[PATCH 10/12] xfs: fully initialise temp leaf in xfs_attr3_leaf_compact
is missing from the branch. Can you commit it, please?
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Hey Dave,
Quoted 34 lines · Dave Chinner
Quoted 24 lines · Ben Myers
Hey,
Quoted 21 lines · Dave Chinner
Quoted 20 lines · Dave Chinner
Hi folks,
This is the latest update of the series of patches tht introduces CRC support into xfsprogs. Of note, for CRC enabled filesystems;
- write support for xfs-db is disabled
- obfuscation for metadump is disabled
-xfs_checkdoes nothing ("always succeed") so that xfstests can run without needing this
- all structures shoul dbe supported for printing inxfs_db
-xfs_repairshould be able to fully validate the structure of a CRC enabled filesystem.
-xfs_repairstill ignores CRC validation errors when reading metadata
- mkfs.xfs enforces limitations on the format of CRC enabled filesystems (inode size, attr format, projid32bit, etc).
- whenever a v5 superblock is parsed on read by any utility, it outputs a wanring about it being an experimental format.Quoted 8 lines
Quoted 5 lines
Quoted 1 line
Bug reports, patches, comments, reviews, etc all welcome.
I've just realised that I haven't ported any of the recent kernel fixes across to this patch set, so there will be another few patches needed for those as well...
These two series are applied to the crc-dev branch at
git://oss.sgi.com/xfs/cmds/xfsprogs.git
Quoted 7 lines
Hi Ben,
Thanks for doing this. This patch, however:
[PATCH 10/12] xfs: fully initialise temp leaf in
xfs_attr3_leaf_compactis missing from the branch. Can you commit it, please?
Ah. Sorry about that. It's applied now.
Regards,
Ben
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
cover xfsprogs: add recent kernel CRC fixes
In reply to Dave ChinnerHi folks,
heres the update with (I think) all of the recent kernel fixes that were missing from the original patchset. These are jsut a straight port of the patches from the kernel tree to libxfs. They should go into the xfsprogs tree at the same time as the parent series.
-Dave.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 1/12 xfs: fix da node magic number mismatches
From: Dave Chinner <[email protected]>
--- libxfs/xfs_da_btree.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
libxfs/xfs_da_btree.c +1 -1
@@ -288,8 +288,8 @@ xfs_da3_node_read(int type;switch (be16_to_cpu (info->magic)) { -case XFS_DA3_NODE_MAGIC :case XFS_DA_NODE_MAGIC : +case XFS_DA3_NODE_MAGIC : type =XFS_BLFT_DA_NODE_BUF ;break ;case XFS_ATTR_LEAF_MAGIC :
Signature · 6 lines
1.7.10.4 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 01/12] xfs: fix da node magic number mismatches
Quoted 3 lines · Dave Chinner
From: Dave Chinner <[email protected]>
Signed-off-by Dave Chinner <[email protected]>
commit cab09a81fbefcb
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 2/12 xfs: Remote attr validation fixes and optimisations
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
- optimise the calcuation for the number of blocks in a remote xattr.
- check attribute length against MAX_XATTR_SIZE, not MAXPATHLEN
- whitespace fixes
--- libxfs/xfs_attr_remote.c | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-)
libxfs/xfs_attr_remote.c +5 -14
@@ -29,15 +29,9 @@ xfs_attr3_rmt_blocks(struct xfs_mount *mp, int attrlen) { - int fsblocks = 0; -int len = attrlen; - -do { - fsblocks++; - len -=XFS_ATTR3_RMT_BUF_SPACE (mp, mp->m_sb.sb_blocksize); - }while (len >0 ); - -return fsblocks; + int buflen = XFS_ATTR3_RMT_BUF_SPACE(mp, + mp->m_sb.sb_blocksize); +return (attrlen + buflen -1 ) / buflen; } static bool
@@ -56,7 +50,7 @@ xfs_attr3_rmt_verify(if (bp->b_bn !=be64_to_cpu (rmt->rm_blkno))return false;if (be32_to_cpu (rmt->rm_offset) + -be32_to_cpu (rmt->rm_bytes) >=MAXPATHLEN ) +be32_to_cpu (rmt->rm_bytes) >=XATTR_SIZE_MAX )return false;if (rmt->rm_owner ==0 )return false;
@@ -160,7 +154,6 @@ xfs_attr3_rmt_hdr_ok(/* ok */ return true; - }/*
@@ -344,7 +337,6 @@ xfs_attr_rmtval_set(* spill for another block every 9 headers we require in this * loop. */ -if (crcs && blkcnt ==0 ) {int total_len;
@@ -399,9 +391,8 @@ xfs_attr_rmtval_set( byte_cnt =BBTOB (bp->b_length); byte_cnt =XFS_ATTR3_RMT_BUF_SPACE (mp, byte_cnt); -if (valuelen < byte_cnt) { +if (valuelen < byte_cnt) byte_cnt = valuelen; - } buf = bp->b_addr; buf += xfs_attr3_rmt_hdr_set(mp, dp->i_ino, offset,
Signature · 6 lines
1.7.10.4 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 02/12] xfs: Remote attr validation fixes and optimisations
Quoted 7 lines · Dave Chinner
From: Dave Chinner <[email protected]>
- optimise the calcuation for the number of blocks in a remote xattr.
- check attribute length againstMAX_XATTR_SIZE, not MAXPATHLEN
- whitespace fixesSigned-off-by Dave Chinner <[email protected]>
commit 946217ba28637d
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 3/12 xfs: xfs_attr_shortform_allfit() does not handle attr3 format.
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
xfstests generic/117 fails with:
XFS: Assertion failed: leaf->hdr.info.magic == cpu_to_be16(XFS_ATTR_LEAF_MAGIC)
indicating a function that does not handle the attr3 format correctly. Fix it.
--- libxfs/xfs_attr_leaf.c | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-)
libxfs/xfs_attr_leaf.c +13 -11
@@ -720,20 +720,22 @@ out:*/ int xfs_attr_shortform_allfit ( -struct xfs_buf *bp, -struct xfs_inode *dp) +struct xfs_buf *bp, +struct xfs_inode *dp) { -xfs_attr_leafblock_t *leaf; -xfs_attr_leaf_entry_t *entry; +struct xfs_attr_leafblock *leaf; +struct xfs_attr_leaf_entry *entry;xfs_attr_leaf_name_local_t *name_loc; -int bytes, i; +struct xfs_attr3_icleaf_hdr leafhdr; +int bytes; +int i; leaf = bp->b_addr; -ASSERT (leaf->hdr.info.magic ==cpu_to_be16 (XFS_ATTR_LEAF_MAGIC )); +xfs_attr3_leaf_hdr_from_disk (&leafhdr, leaf); + entry =xfs_attr3_leaf_entryp (leaf); - entry = &leaf->entries[0 ]; bytes =sizeof (struct xfs_attr_sf_hdr ); -for (i =0 ; i <be16_to_cpu (leaf->hdr.count); entry++, i++) { +for (i =0 ; i < leafhdr.count; entry++, i++) {if (entry->flags &XFS_ATTR_INCOMPLETE )continue ;/* don't copy partial entries */ if (!(entry->flags &XFS_ATTR_LOCAL ))
@@ -743,15 +745,15 @@ xfs_attr_shortform_allfit(return (0 );if (be16_to_cpu (name_loc->valuelen) >=XFS_ATTR_SF_ENTSIZE_MAX )return (0 ); - bytes +=sizeof (struct xfs_attr_sf_entry )-1 + bytes +=sizeof (struct xfs_attr_sf_entry ) -1 + name_loc->namelen +be16_to_cpu (name_loc->valuelen); }if ((dp->i_mount->m_flags &XFS_MOUNT_ATTR2 ) && (dp->i_d.di_format !=XFS_DINODE_FMT_BTREE ) && (bytes ==sizeof (struct xfs_attr_sf_hdr ))) -return (-1 ); -return (xfs_attr_shortform_bytesfit (dp, bytes)); +return -1 ; +return xfs_attr_shortform_bytesfit (dp, bytes); }/*
Signature · 6 lines
1.7.10.4 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 03/12] xfs: xfs_attr_shortform_allfit() does not handle attr3 format.
Quoted 10 lines · Dave Chinner
From: Dave Chinner <[email protected]>
xfstests generic/117 fails with:
XFS: Assertion failed:
leaf->hdr.info.magic==cpu_to_be16(XFS_ATTR_LEAF_MAGIC)indicating a function that does not handle the attr3 format correctly. Fix it.
Signed-off-by Dave Chinner <[email protected]>
Goes with commit b38958d7153160
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 4/12 xfs: remote attribute lookups require the value length
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
When reading a remote attribute, to correctly calculate the length of the data buffer for CRC enable filesystems, we need to know the length of the attribute data. We get this information when we look up the attribute, but we don't store it in the args structure along with the other remote attr information we get from the lookup. Add this information to the args structure so we can use it appropriately.
--- libxfs/xfs_attr_leaf.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
libxfs/xfs_attr_leaf.c +2 -1
@@ -2122,9 +2122,10 @@ xfs_attr3_leaf_lookup_int(if (!xfs_attr_namesp_match (args->flags, entry->flags))continue ; args->index = probe; + args->valuelen =be32_to_cpu (name_rmt->valuelen); args->rmtblkno =be32_to_cpu (name_rmt->valueblk); args->rmtblkcnt =XFS_B_TO_FSB (args->dp->i_mount, -be32_to_cpu (name_rmt->valuelen)); + args->valuelen);return XFS_ERROR (EEXIST ); } }
Signature · 6 lines
1.7.10.4 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 04/12] xfs: remote attribute lookups require the value length
Quoted 11 lines · Dave Chinner
From: Dave Chinner <[email protected]>
When reading a remote attribute, to correctly calculate the length of the data buffer for CRC enable filesystems, we need to know the length of the attribute data. We get this information when we look up the attribute, but we don't store it in the args structure along with the other remote attr information we get from the lookup. Add this information to the args structure so we can use it appropriately.
Signed-off-by Dave Chinner <[email protected]>
Goes with commit e461fcb194172b3f709e0b478d2ac1bdac7ab9a3
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 5/12 xfs: remote attribute allocation may be contiguous
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
When CRCs are enabled, there may be multiple allocations made if the headers cause a length overflow. This, however, does not mean that the number of headers required increases, as the second and subsequent extents may be contiguous with the previous extent. Hence when we map the extents to write the attribute data, we may end up with less extents than allocations made. Hence the assertion that we consume th enumber of headers we calculated in the allocation loop is incorrect and needs to be removed.
--- libxfs/xfs_attr_remote.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
libxfs/xfs_attr_remote.c +5 -1
@@ -336,6 +336,11 @@ xfs_attr_rmtval_set(* into requiring more blocks. e.g. for 512 byte blocks, we'll * spill for another block every 9 headers we require in this * loop. +* +* Note that this can result in contiguous allocation of blocks, +* so we don't use all the space we allocate for headers as we +* have one less header for each contiguous allocation that +* occurs in the map/write loop below. */ if (crcs && blkcnt ==0 ) {int total_len;
@@ -416,7 +421,6 @@ xfs_attr_rmtval_set( lblkno += map.br_blockcount; }ASSERT (valuelen ==0 ); -ASSERT (hdrcnt ==0 );return 0 ; }
Signature · 6 lines
1.7.10.4 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 05/12] xfs: remote attribute allocation may be contiguous
Quoted 11 lines · Dave Chinner
From: Dave Chinner <[email protected]>
When CRCs are enabled, there may be multiple allocations made if the headers cause a length overflow. This, however, does not mean that the number of headers required increases, as the second and subsequent extents may be contiguous with the previous extent. Hence when we map the extents to write the attribute data, we may end up with less extents than allocations made. Hence the assertion that we consume th enumber of headers we calculated in the allocation loop is incorrect and needs to be removed.
Quoted 1 line
Signed-off-by Dave Chinner <[email protected]>
Goes with commit 90253cf142469a40f89f989904abf0a1e500e1a6
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 6/12 xfs: remote attribute read too short
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
Reading a maximally size remote attribute fails when CRCs are enabled with this verification error:
XFS (vdb): remote attribute header does not match required off/len/owner)
There are two reasons for this, the first being that the length of the buffer being read is determined from the args->rmtblkcnt which doesn't take into account CRC headers. Hence the mapped length ends up being too short and so we need to calculate it directly from the value length.
The second is that the byte count of valid data within a buffer is capped by the length of the data and so doesn't take into account that the buffer might be longer due to headers. Hence we need to calculate the data space in the buffer first before calculating the actual byte count of data.
--- libxfs/xfs_attr_remote.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)
libxfs/xfs_attr_remote.c +9 -6
@@ -29,9 +29,11 @@ xfs_attr3_rmt_blocks( struct xfs_mount *mp, int attrlen) { - int buflen = XFS_ATTR3_RMT_BUF_SPACE(mp, - mp->m_sb.sb_blocksize); -return (attrlen + buflen -1 ) / buflen; +if (xfs_sb_version_hascrc (&mp->m_sb)) { +int buflen =XFS_ATTR3_RMT_BUF_SPACE (mp, mp->m_sb.sb_blocksize); +return (attrlen + buflen -1 ) / buflen; + } +return XFS_B_TO_FSB (mp, attrlen); } static bool
@@ -183,8 +185,9 @@ xfs_attr_rmtval_get(while (valuelen >0 ) { nmap =ATTR_RMTVALUE_MAPSIZE ; + blkcnt =xfs_attr3_rmt_blocks (mp, valuelen); error =xfs_bmapi_read (args->dp, (xfs_fileoff_t )lblkno, - args->rmtblkcnt, map, &nmap, + blkcnt, map, &nmap,XFS_BMAPI_ATTRFORK );if (error)return error;
@@ -204,8 +207,8 @@ xfs_attr_rmtval_get(if (error)return error; - byte_cnt =min_t (int, valuelen,BBTOB (bp->b_length)); - byte_cnt =XFS_ATTR3_RMT_BUF_SPACE (mp, byte_cnt); + byte_cnt =XFS_ATTR3_RMT_BUF_SPACE (mp,BBTOB (bp->b_length)); + byte_cnt =min_t (int, valuelen, byte_cnt); src = bp->b_addr;if (xfs_sb_version_hascrc (&mp->m_sb)) {
Signature · 6 lines
1.7.10.4 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 06/12] xfs: remote attribute read too short
Quoted 13 lines · Dave Chinner
From: Dave Chinner <[email protected]>
Reading a maximally size remote attribute fails when CRCs are enabled with this verification error:
XFS (vdb): remote attribute header does not match required off/len/owner)
There are two reasons for this, the first being that the length of the buffer being read is determined from the
args->rmtblkcntwhich doesn't take into account CRC headers. Hence the mapped length ends up being too short and so we need to calculate it directly from the value length.
Quoted 7 lines
The second is that the byte count of valid data within a buffer is capped by the length of the data and so doesn't take into account that the buffer might be longer due to headers. Hence we need to calculate the data space in the buffer first before calculating the actual byte count of data.
Signed-off-by Dave Chinner <[email protected]>
Goes with commit 551b382f5368900d6d82983505cb52553c946a2b
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 7/12 xfs: remote attribute tail zeroing does too much
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
When an attribute data does not fill then entire remote block, we zero the remaining part of the buffer. This, however, needs to take into account that the buffer has a header, and so the offset where zeroing starts and the length of zeroing need to take this into account. Otherwise we end up with zeros over the end of the attribute value when CRCs are enabled.
While there, make sure we only ask to map an extent that covers the remaining range of the attribute, rather than asking every time for the full length of remote data. If the remote attribute blocks are contiguous with other parts of the attribute tree, it will map those blocks as well and we can potentially zero them incorrectly. We can also get buffer size mistmatches when trying to read or remove the remote attribute, and this can lead to not finding the correct buffer when looking it up in cache.
--- libxfs/xfs_attr_remote.c | 35 +++++++++++++++++------------------ 1 file changed, 17 insertions(+), 18 deletions(-)
libxfs/xfs_attr_remote.c +17 -18
@@ -273,10 +273,7 @@ xfs_attr_rmtval_set(* and we may not need that many, so we have to handle this when * allocating the blocks below. */ -if (!crcs) - blkcnt =XFS_B_TO_FSB (mp, args->valuelen); -else - blkcnt =xfs_attr3_rmt_blocks (mp, args->valuelen); + blkcnt =xfs_attr3_rmt_blocks (mp, args->valuelen); error =xfs_bmap_first_unused (args->trans, args->dp, blkcnt, &lfileoff,XFS_ATTR_FORK );
@@ -371,8 +368,11 @@ xfs_attr_rmtval_set(*/ lblkno = args->rmtblkno; valuelen = args->valuelen; + blkcnt = args->rmtblkcnt;while (valuelen >0 ) {int byte_cnt; +int hdr_size; +int dblkcnt;char *buf;/*
@@ -381,7 +381,7 @@ xfs_attr_rmtval_set(xfs_bmap_init (args->flist, args->firstblock); nmap =1 ; error =xfs_bmapi_read (dp, (xfs_fileoff_t )lblkno, - args->rmtblkcnt, &map, &nmap, + blkcnt, &map, &nmap,XFS_BMAPI_ATTRFORK );if (error)return (error);
@@ -390,26 +390,25 @@ xfs_attr_rmtval_set( (map.br_startblock != HOLESTARTBLOCK)); dblkno =XFS_FSB_TO_DADDR (mp, map.br_startblock), - blkcnt =XFS_FSB_TO_BB (mp, map.br_blockcount); + dblkcnt =XFS_FSB_TO_BB (mp, map.br_blockcount); - bp =xfs_buf_get (mp->m_ddev_targp, dblkno, blkcnt,0 ); + bp =xfs_buf_get (mp->m_ddev_targp, dblkno, dblkcnt,0 );if (!bp)return ENOMEM ; bp->b_ops = &xfs_attr3_rmt_buf_ops; - byte_cnt =BBTOB (bp->b_length); - byte_cnt =XFS_ATTR3_RMT_BUF_SPACE (mp, byte_cnt); -if (valuelen < byte_cnt) - byte_cnt = valuelen; - buf = bp->b_addr; - buf +=xfs_attr3_rmt_hdr_set (mp, dp->i_ino, offset, + byte_cnt =XFS_ATTR3_RMT_BUF_SPACE (mp,BBTOB (bp->b_length)); + byte_cnt =min_t (int, valuelen, byte_cnt); + hdr_size =xfs_attr3_rmt_hdr_set (mp, dp->i_ino, offset, byte_cnt, bp); -memcpy (buf, src, byte_cnt); +ASSERT (hdr_size + byte_cnt <=BBTOB (bp->b_length)); + +memcpy (buf + hdr_size, src, byte_cnt); -if (byte_cnt <BBTOB (bp->b_length)) -xfs_buf_zero (bp, byte_cnt, -BBTOB (bp->b_length) - byte_cnt); +if (byte_cnt + hdr_size <BBTOB (bp->b_length)) +xfs_buf_zero (bp, byte_cnt + hdr_size, +BBTOB (bp->b_length) - byte_cnt - hdr_size); error =xfs_bwrite (bp);/* GROT: NOTE: synchronous write */ xfs_buf_relse (bp);
@@ -419,9 +418,9 @@ xfs_attr_rmtval_set( src += byte_cnt; valuelen -= byte_cnt; offset += byte_cnt; - hdrcnt--; lblkno += map.br_blockcount; + blkcnt -= map.br_blockcount; }ASSERT (valuelen ==0 );return 0 ;
Signature · 6 lines
1.7.10.4 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 07/12] xfs: remote attribute tail zeroing does too much
Quoted 18 lines · Dave Chinner
From: Dave Chinner <[email protected]>
When an attribute data does not fill then entire remote block, we zero the remaining part of the buffer. This, however, needs to take into account that the buffer has a header, and so the offset where zeroing starts and the length of zeroing need to take this into account. Otherwise we end up with zeros over the end of the attribute value when CRCs are enabled.
While there, make sure we only ask to map an extent that covers the remaining range of the attribute, rather than asking every time for the full length of remote data. If the remote attribute blocks are contiguous with other parts of the attribute tree, it will map those blocks as well and we can potentially zero them incorrectly. We can also get buffer size mistmatches when trying to read or remove the remote attribute, and this can lead to not finding the correct buffer when looking it up in cache.
Quoted 1 line
Signed-off-by Dave Chinner <[email protected]>
Goes with commit 4af3644c9a53eb2f1ecf69cc53576561b64be4c6
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 8/12 xfs: correctly map remote attr buffers during removal
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
If we don't map the buffers correctly (same as for get/set operations) then the incore buffer lookup will fail. If a block number matches but a length is wrong, then debug kernels will ASSERT fail in _xfs_buf_find() due to the length mismatch. Ensure that we map the buffers correctly by basing the length of the buffer on the attribute data length rather than the remote block count.
--- libxfs/xfs_attr_remote.c | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-)
libxfs/xfs_attr_remote.c +18 -9
@@ -445,19 +445,25 @@ xfs_attr_rmtval_remove(xfs_da_args_t *args) mp = args->dp->i_mount;/* -* Roll through the "value", invalidating the attribute value's -* blocks. +* Roll through the "value", invalidating the attribute value's blocks. +* Note that args->rmtblkcnt is the minimum number of data blocks we'll +* see for a CRC enabled remote attribute. Each extent will have a +* header, and so we may have more blocks than we realise here. If we +* fail to map the blocks correctly, we'll have problems with the buffer +* lookups. */ lblkno = args->rmtblkno; - valuelen = args->rmtblkcnt; + valuelen = args->valuelen; + blkcnt =xfs_attr3_rmt_blocks (mp, valuelen);while (valuelen >0 ) { +int dblkcnt; +/* * Try to remember where we decided to put the value. */ nmap =1 ; error =xfs_bmapi_read (args->dp, (xfs_fileoff_t )lblkno, - args->rmtblkcnt, &map, &nmap, -XFS_BMAPI_ATTRFORK ); + blkcnt, &map, &nmap,XFS_BMAPI_ATTRFORK );if (error)return (error);ASSERT (nmap ==1 );
@@ -465,28 +471,31 @@ xfs_attr_rmtval_remove(xfs_da_args_t *args) (map.br_startblock != HOLESTARTBLOCK)); dblkno =XFS_FSB_TO_DADDR (mp, map.br_startblock), - blkcnt =XFS_FSB_TO_BB (mp, map.br_blockcount); + dblkcnt =XFS_FSB_TO_BB (mp, map.br_blockcount);/* * If the "remote" value is in the cache, remove it. */ - bp =xfs_incore (mp->m_ddev_targp, dblkno, blkcnt,XBF_TRYLOCK ); + bp =xfs_incore (mp->m_ddev_targp, dblkno, dblkcnt,XBF_TRYLOCK );if (bp) {xfs_buf_stale (bp);xfs_buf_relse (bp); bp =NULL ; } - valuelen -= map.br_blockcount; + valuelen -=XFS_ATTR3_RMT_BUF_SPACE (mp, +XFS_FSB_TO_B (mp, map.br_blockcount)); lblkno += map.br_blockcount; + blkcnt -= map.br_blockcount; + blkcnt =max (blkcnt,xfs_attr3_rmt_blocks (mp, valuelen)); }/* * Keep de-allocating extents until the remote-value region is gone. */ + blkcnt = lblkno - args->rmtblkno; lblkno = args->rmtblkno; - blkcnt = args->rmtblkcnt; done =0 ;while (!done) {xfs_bmap_init (args->flist, args->firstblock);
Signature · 6 lines
1.7.10.4 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 08/12] xfs: correctly map remote attr buffers during removal
Quoted 10 lines · Dave Chinner
From: Dave Chinner <[email protected]>
If we don't map the buffers correctly (same as for get/set operations) then the incore buffer lookup will fail. If a block number matches but a length is wrong, then debug kernels will ASSERT fail in
_xfs_buf_find()due to the length mismatch. Ensure that we map the buffers correctly by basing the length of the buffer on the attribute data length rather than the remote block count.Signed-off-by Dave Chinner <[email protected]>
Goes with commit 6863ef8449f1908c19f43db572e4474f24a1e9da
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 9/12 xfs: fully initialise temp leaf in xfs_attr3_leaf_unbalance
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
xfs_attr3_leaf_unbalance() uses a temporary buffer for recombining the entries in two leaves when the destination leaf requires compaction. The temporary buffer ends up being copied back over the original destination buffer, so the header in the temporary buffer needs to contain all the information that is in the destination buffer.
To make sure the temporary buffer is fully initialised, once we've set up the temporary incore header appropriately, write is back to the temporary buffer before starting to move entries around.
--- libxfs/xfs_attr_leaf.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)
libxfs/xfs_attr_leaf.c +13 -3
@@ -1971,14 +1971,24 @@ xfs_attr3_leaf_unbalance(struct xfs_attr_leafblock *tmp_leaf;struct xfs_attr3_icleaf_hdr tmphdr; - tmp_leaf =kmem_alloc (state->blocksize,KM_SLEEP ); -memset (tmp_leaf,0 , state->blocksize); -memset (&tmphdr,0 ,sizeof (tmphdr)); + tmp_leaf =kmem_zalloc (state->blocksize,KM_SLEEP ); + +/* +* Copy the header into the temp leaf so that all the stuff +* not in the incore header is present and gets copied back in +* once we've moved all the entries. +*/ +memcpy (tmp_leaf, save_leaf,xfs_attr3_leaf_hdr_size (save_leaf)); +memset (&tmphdr,0 ,sizeof (tmphdr)); tmphdr.magic = savehdr.magic; tmphdr.forw = savehdr.forw; tmphdr.back = savehdr.back; tmphdr.firstused = state->blocksize; + +/* write the header to the temp buffer to initialise it */ +xfs_attr3_leaf_hdr_to_disk (tmp_leaf, &tmphdr); +if (xfs_attr3_leaf_order (save_blk->bp, &savehdr, drop_blk->bp, &drophdr)) { xfs_attr3_leaf_moveents(drop_leaf, &drophdr, 0,
Signature · 6 lines
1.7.10.4 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 09/12] xfs: fully initialise temp leaf in xfs_attr3_leaf_unbalance
Quoted 9 lines · Dave Chinner
From: Dave Chinner <[email protected]>
xfs_attr3_leaf_unbalance()uses a temporary buffer for recombining the entries in two leaves when the destination leaf requires compaction. The temporary buffer ends up being copied back over the original destination buffer, so the header in the temporary buffer needs to contain all the information that is in the destination buffer.
Quoted 5 lines
To make sure the temporary buffer is fully initialised, once we've set up the temporary incore header appropriately, write is back to the temporary buffer before starting to move entries around.
Signed-off-by Dave Chinner <[email protected]>
Goes with commit 8517de2a81da830f5d90da66b4799f4040c76dc9
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 10/12 xfs: fully initialise temp leaf in xfs_attr3_leaf_compact
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
xfs_attr3_leaf_compact() uses a temporary buffer for compacting the the entries in a leaf. It copies the the original buffer into the temporary buffer, then zeros the original buffer completely. It then copies the entries back into the original buffer. However, the original buffer has not been correctly initialised, and so the movement of the entries goes horribly wrong.
Make sure the zeroed destination buffer is fully initialised, and once we've set up the destination incore header appropriately, write is back to the buffer before starting to move entries around.
While debugging this, the _d/_s prefixes weren't sufficient to remind me what buffer was what, so rename then all _src/_dst.
--- libxfs/xfs_attr_leaf.c | 42 ++++++++++++++++++++++++++---------------- 1 file changed, 26 insertions(+), 16 deletions(-)
libxfs/xfs_attr_leaf.c +26 -16
@@ -1235,11 +1235,12 @@ xfs_attr3_leaf_add_work( STATIC voidxfs_attr3_leaf_compact (struct xfs_da_args *args, -struct xfs_attr3_icleaf_hdr *ichdr_d, +struct xfs_attr3_icleaf_hdr *ichdr_dst,struct xfs_buf *bp) { -xfs_attr_leafblock_t *leaf_s, *leaf_d; -struct xfs_attr3_icleaf_hdr ichdr_s; +struct xfs_attr_leafblock *leaf_src; +struct xfs_attr_leafblock *leaf_dst; +struct xfs_attr3_icleaf_hdr ichdr_src;struct xfs_trans *trans = args->trans;struct xfs_mount *mp = trans->t_mountp;char *tmpbuffer;
@@ -1247,29 +1248,38 @@ xfs_attr3_leaf_compact(trace_xfs_attr_leaf_compact (args); tmpbuffer =kmem_alloc (XFS_LBSIZE (mp),KM_SLEEP ); -ASSERT (tmpbuffer !=NULL );memcpy (tmpbuffer, bp->b_addr,XFS_LBSIZE (mp));memset (bp->b_addr,0 ,XFS_LBSIZE (mp)); + leaf_src = (xfs_attr_leafblock_t *)tmpbuffer; + leaf_dst = bp->b_addr;/* -* Copy basic information +* Copy the on-disk header back into the destination buffer to ensure +* all the information in the header that is not part of the incore +* header structure is preserved. */ - leaf_s = (xfs_attr_leafblock_t *)tmpbuffer; - leaf_d = bp->b_addr; - ichdr_s = *ichdr_d;/* struct copy */ - ichdr_d->firstused =XFS_LBSIZE (mp); - ichdr_d->usedbytes =0 ; - ichdr_d->count =0 ; - ichdr_d->holes =0 ; - ichdr_d->freemap[0 ].base =xfs_attr3_leaf_hdr_size (leaf_s); - ichdr_d->freemap[0 ].size = ichdr_d->firstused - ichdr_d->freemap[0 ].base; +memcpy (bp->b_addr, tmpbuffer,xfs_attr3_leaf_hdr_size (leaf_src)); + +/* Initialise the incore headers */ + ichdr_src = *ichdr_dst;/* struct copy */ + ichdr_dst->firstused =XFS_LBSIZE (mp); + ichdr_dst->usedbytes =0 ; + ichdr_dst->count =0 ; + ichdr_dst->holes =0 ; + ichdr_dst->freemap[0 ].base =xfs_attr3_leaf_hdr_size (leaf_src); + ichdr_dst->freemap[0 ].size = ichdr_dst->firstused - + ichdr_dst->freemap[0 ].base; + + +/* write the header back to initialise the underlying buffer */ +xfs_attr3_leaf_hdr_to_disk (leaf_dst, ichdr_dst);/* * Copy all entry's in the same (sorted) order, * but allocate name/value pairs packed and in sequence. */ -xfs_attr3_leaf_moveents (leaf_s, &ichdr_s,0 , leaf_d, ichdr_d,0 , - ichdr_s.count, mp); +xfs_attr3_leaf_moveents (leaf_src, &ichdr_src,0 , leaf_dst, ichdr_dst,0 , + ichdr_src.count, mp);/* * this logs the entire buffer, but the caller must write the header * back to the buffer when it is finished modifying it.
Signature · 6 lines
1.7.10.4 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 10/12] xfs: fully initialise temp leaf in xfs_attr3_leaf_compact
Quoted 9 lines · Dave Chinner
From: Dave Chinner <[email protected]>
xfs_attr3_leaf_compact()uses a temporary buffer for compacting the the entries in a leaf. It copies the the original buffer into the temporary buffer, then zeros the original buffer completely. It then copies the entries back into the original buffer. However, the original buffer has not been correctly initialised, and so the movement of the entries goes horribly wrong.
Quoted 8 lines
Make sure the zeroed destination buffer is fully initialised, and once we've set up the destination incore header appropriately, write is back to the buffer before starting to move entries around.
While debugging this, the _d/_s prefixes weren't sufficient to remind me what buffer was what, so rename then all _src/_dst.
Signed-off-by Dave Chinner <[email protected]>
Goes with commit d4c712bcf26a25c2b67c90e44e0b74c7993b5334
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 11/12 xfs: rework remote attr CRCs
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
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.
--- fsr/xfs_fsr.c | 1 - include/libxfs.h | 1 + include/libxlog.h | 1 - include/xfs_attr_remote.h | 10 ++ libxfs/xfs_attr_leaf.c | 10 +- libxfs/xfs_attr_remote.c | 387 ++++++++++++++++++++++++++------------------- 6 files changed, 245 insertions(+), 165 deletions(-)
fsr/xfs_fsr.c +0 -1
@@ -77,7 +77,6 @@ static __int64_t minimumfree = 2048;#define V_ALL 2#define BUFFER_SIZE (1<<16)#define BUFFER_MAX (1<<24) -#define min (x, y) ((x) < (y) ? (x) : (y))static time_t howlong =7200 ;/* default seconds of reorganizing */ static char *leftofffile = _PATH_FSRLAST;/* where we left off last */
include/libxfs.h +1 -0
@@ -71,6 +71,7 @@#define __round_mask (x, y) ((__typeof__(x))((y)-1))#define round_up (x, y) ((((x)-1) | __round_mask(x, y))+1)#define round_down (x, y) ((x) & ~__round_mask(x, y)) +#define min (a,b) ((a) < (b) ? (a) : (b))/* * Argument structure for libxfs_init().
include/libxlog.h +0 -1
@@ -74,7 +74,6 @@ typedef union {#define XFS_CORRUPTION_ERROR (e,l,mp,m) ((void) 0)#define XFS_MOUNT_WAS_CLEAN 0x1#define unlikely (x) (x) -#define min (a,b) ((a) < (b) ? (a) : (b)) #define xfs_warn(mp,fmt,args...) cmn_err(CE_WARN,fmt, ## args) #define xfs_alert(mp,fmt,args...) cmn_err(CE_ALERT,fmt, ## args)
include/xfs_attr_remote.h +10 -0
@@ -26,6 +26,14 @@#define XFS_ATTR3_RMT_MAGIC 0x5841524d/* XARM */ +/* +* There is one of these headers per filesystem block in a remote attribute. +* This is done to ensure there is a 1:1 mapping between the attribute value +* length and the number of blocks needed to store the attribute. This makes the +* verification of a buffer a little more complex, but greatly simplifies the +* allocation, reading and writing of these attributes as we don't have to guess +* the number of blocks needed to store the attribute data. +*/ struct xfs_attr3_rmt_hdr {__be32 rm_magic;__be32 rm_offset;
@@ -45,6 +53,8 @@ struct xfs_attr3_rmt_hdr {extern const struct xfs_buf_ops xfs_attr3_rmt_buf_ops; +int xfs_attr3_rmt_blocks (struct xfs_mount *mp,int attrlen); +int xfs_attr_rmtval_get (struct xfs_da_args *args);int xfs_attr_rmtval_set (struct xfs_da_args *args);int xfs_attr_rmtval_remove (struct xfs_da_args *args);
libxfs/xfs_attr_leaf.c +6 -4
@@ -1202,7 +1202,7 @@ xfs_attr3_leaf_add_work( name_rmt->valuelen =0 ; name_rmt->valueblk =0 ; args->rmtblkno =1 ; - args->rmtblkcnt =XFS_B_TO_FSB (mp, args->valuelen); + args->rmtblkcnt =xfs_attr3_rmt_blocks (mp, args->valuelen); } xfs_trans_log_buf(args->trans, bp, XFS_DA_LOGRANGE(leaf, xfs_attr3_leaf_name(leaf, args->index),
@@ -2144,8 +2144,9 @@ xfs_attr3_leaf_lookup_int( args->index = probe; args->valuelen =be32_to_cpu (name_rmt->valuelen); args->rmtblkno =be32_to_cpu (name_rmt->valueblk); - args->rmtblkcnt =XFS_B_TO_FSB (args->dp->i_mount, - args->valuelen); + args->rmtblkcnt =xfs_attr3_rmt_blocks ( + args->dp->i_mount, + args->valuelen);return XFS_ERROR (EEXIST ); } }
@@ -2196,7 +2197,8 @@ xfs_attr3_leaf_getvalue(ASSERT (memcmp (args->name, name_rmt->name, args->namelen) ==0 ); valuelen =be32_to_cpu (name_rmt->valuelen); args->rmtblkno =be32_to_cpu (name_rmt->valueblk); - args->rmtblkcnt =XFS_B_TO_FSB (args->dp->i_mount, valuelen); + args->rmtblkcnt =xfs_attr3_rmt_blocks (args->dp->i_mount, + valuelen);if (args->flags &ATTR_KERNOVAL ) { args->valuelen = valuelen;return 0 ;
libxfs/xfs_attr_remote.c +228 -159
581 lines not on this page · raw message
Signature · 6 lines
1.7.10.4 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 11/12] xfs: rework remote attr CRCs
Quoted 60 lines · Dave Chinner
From: Dave Chinner <[email protected]>
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.
Quoted 1 line
Signed-off-by Dave Chinner <[email protected]>
Goes with commit ad1858d77771172e08016890f0eb2faedec3ecee
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 12/12 xfs: don't emit v5 superblock warnings on write
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
We write the superblock every 30s or so which results in the verifier being called. Right now that results in this output every 30s:
XFS (vda): Version 5 superblock detected. This kernel has EXPERIMENTAL support enabled! Use of these features in this kernel is at your own risk!
And spamming the logs.
We don't need to check for whether we support v5 superblocks or whether there are feature bits we don't support set as these are only relevant when we first mount the filesytem. i.e. on superblock read. Hence for the write verification we can just skip all the checks (and hence verbose output) altogether.
--- libxfs/xfs_mount.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
libxfs/xfs_mount.c +6 -5
@@ -120,7 +120,8 @@ STATIC intxfs_mount_validate_sb ( xfs_mount_t *mp, xfs_sb_t *sbp, - bool check_inprogress) + bool check_inprogress, + bool check_version) {/*
@@ -145,7 +146,7 @@ xfs_mount_validate_sb(* Version 5 superblock feature mask validation. Reject combinations the * kernel cannot support up front before checking anything else. */ -if (check_inprogress &&XFS_SB_VERSION_NUM (sbp) ==XFS_SB_VERSION_5 ) { +if (check_version &&XFS_SB_VERSION_NUM (sbp) ==XFS_SB_VERSION_5 ) {xfs_alert (mp,"Version 5 superblock detected. xfsprogs has EXPERIMENTAL support enabled!\n" "Use of these features is at your own risk!" );
@@ -370,7 +371,7 @@ xfs_sb_to_disk(static int xfs_sb_verify (struct xfs_buf *bp, -bool verbose) +bool check_version) {struct xfs_mount *mp = bp->b_target->bt_mount;struct xfs_sb sb;
@@ -381,8 +382,8 @@ xfs_sb_verify(* Only check the in progress field for the primary superblock as * mkfs.xfs doesn't clear it from secondary superblocks. */ -return xfs_mount_validate_sb (mp, &sb, - verbose && bp->b_bn ==XFS_SB_DADDR ); +return xfs_mount_validate_sb (mp, &sb, bp->b_bn ==XFS_SB_DADDR , + check_version); }/*
Signature · 6 lines
1.7.10.4 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 12/12] xfs: don't emit v5 superblock warnings on write
Quoted 11 lines · Dave Chinner
From: Dave Chinner <[email protected]>
We write the superblock every 30s or so which results in the verifier being called. Right now that results in this output every 30s:
XFS (vda): Version 5 superblock detected. This kernel has EXPERIMENTAL support enabled! Use of these features in this kernel is at your own risk!
And spamming the logs.
Quoted 7 lines
We don't need to check for whether we support v5 superblocks or whether there are feature bits we don't support set as these are only relevant when we first mount the filesytem. i.e. on superblock read. Hence for the write verification we can just skip all the checks (and hence verbose output) altogether.
Signed-off-by Dave Chinner <[email protected]>
Goes with commit 34510185abeaa5be9b178a41c0a03d30aec3db7e
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 20 lines · Dave Chinner
Hi folks,
This is the latest update of the series of patches tht introduces CRC support into xfsprogs. Of note, for CRC enabled filesystems;
- write support for xfs-db is disabled
- obfuscation for metadump is disabled
-xfs_checkdoes nothing ("always succeed") so that xfstests can run without needing this
- all structures shoul dbe supported for printing inxfs_db
-xfs_repairshould be able to fully validate the structure of a CRC enabled filesystem.
-xfs_repairstill ignores CRC validation errors when reading metadata
- mkfs.xfs enforces limitations on the format of CRC enabled filesystems (inode size, attr format, projid32bit, etc).
- whenever a v5 superblock is parsed on read by any utility, it outputs a wanring about it being an experimental format.
Quoted 1 line
Bug reports, patches, comments, reviews, etc all welcome.
Pulled in 1-48 of the first series and 1-12 of the second.
-Ben
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
cover xfsprogs: various issues from review
Hey,
Quoted 21 lines · Ben Myers
Quoted 20 lines · Dave Chinner
Hi folks,
This is the latest update of the series of patches tht introduces CRC support into xfsprogs. Of note, for CRC enabled filesystems;
- write support for xfs-db is disabled
- obfuscation for metadump is disabled
-xfs_checkdoes nothing ("always succeed") so that xfstests can run without needing this
- all structures shoul dbe supported for printing inxfs_db
-xfs_repairshould be able to fully validate the structure of a CRC enabled filesystem.
-xfs_repairstill ignores CRC validation errors when reading metadata
- mkfs.xfs enforces limitations on the format of CRC enabled filesystems (inode size, attr format, projid32bit, etc).
- whenever a v5 superblock is parsed on read by any utility, it outputs a wanring about it being an experimental format.
Quoted 3 lines
Quoted 1 line
Bug reports, patches, comments, reviews, etc all welcome.
Pulled in 1-48 of the first series and 1-12 of the second.
Here is a patch series that addresses some of my outstanding concerns from review. Some may be already fixed in the 2nd series, I'm not sure. Eric also mentioned that he put the updated branch through coverity and found some defects. There may be some overlap there too.
Some of these are just reminders for myself to make sure certain items are addressed eventually. Sorry for the noise.
-Ben
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 1/14] libxfs: don't verify bmbt reads twice
xfs_bmbt_read_verify is calling xfs_bmbt_verify twice in a row. commit ee1a47ab0e in the kernel removed the first xfs_bmbt_verify but this was not carried over when it was implemented in userspace.
--- libxfs/xfs_bmap_btree.c | 2 -- 1 file changed, 2 deletions(-)
libxfs/xfs_bmap_btree.c +0 -2
@@ -759,7 +759,6 @@ static voidxfs_bmbt_read_verify ( struct xfs_buf *bp) { -xfs_bmbt_verify (bp);if (!(xfs_btree_lblock_verify_crc (bp) &&xfs_bmbt_verify (bp))) {trace_xfs_btree_corrupt (bp, _RET_IP_);
@@ -767,7 +766,6 @@ xfs_bmbt_read_verify( bp->b_target->bt_mount, bp->b_addr);xfs_buf_ioerror (bp,EFSCORRUPTED ); } - } static void
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 2/14 xfsprogs: XFS_AGF_NUM_BITS should be 13
In reply to Ben Myerscommit 4e0e6040c4052aff15a494ac05778f4086d24c33 changed XFS_AGF_NUM_BITS to be 13, however when this commit was applied to userspace the change was not pulled over.
--- include/xfs_ag.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
include/xfs_ag.h +1 -1
@@ -102,7 +102,7 @@ typedef struct xfs_agf {#define XFS_AGF_LONGEST 0x00000400#define XFS_AGF_BTREEBLKS 0x00000800#define XFS_AGF_UUID 0x00001000 -#define XFS_AGF_NUM_BITS 12 +#define XFS_AGF_NUM_BITS 13#define XFS_AGF_ALL_BITS ((1 << XFS_AGF_NUM_BITS) - 1)#define XFS_AGF_FLAGS \
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 3/14 xfsprogs: pull in the rest of 93848a999cf
In reply to Ben MyersXXX Maybe I missed some more...
--- libxfs/xfs_ialloc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
libxfs/xfs_ialloc.c +1 -1
@@ -204,7 +204,7 @@ xfs_ialloc_inode_init(* individual transactions causing a lot of log traffic. */ fbuf->b_ops = &xfs_inode_buf_ops; -xfs_buf_zero (fbuf,0 , ninodes << mp->m_sb.sb_inodelog); +xfs_buf_zero (fbuf,0 ,BBTOB (fbuf->b_length));for (i =0 ; i < ninodes; i++) {int ioffset = i << mp->m_sb.sb_inodelog;uint isize =xfs_dinode_size (version);
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
***** SUSPECTED SPAM ***** Re: [PATCH 3/14] xfsprogs: pull in the rest of 93848a999cf
Quoted 11 lines · Ben Myers
XXX Maybe I missed some more...
--- libxfs/xfs_ialloc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)Index:
b/libxfs/xfs_ialloc.c
===================================================================
libxfs/xfs_ialloc.c xfs_ialloc_inode_init( · line 210
Quoted 8 lines
libxfs/xfs_ialloc.c +1
-1@@ -204,7 +204,7 @@ xfs_ialloc_inode_init(* individual transactions causing a lot of log traffic. */ fbuf->b_ops = &xfs_inode_buf_ops; -xfs_buf_zero (fbuf,0 , ninodes << mp->m_sb.sb_inodelog); +xfs_buf_zero (fbuf,0 ,BBTOB (fbuf->b_length));for (i =0 ; i < ninodes; i++) {int ioffset = i << mp->m_sb.sb_inodelog;uint isize =xfs_dinode_size (version);
It's fixed in the second series of patches, along with all the other little differences between the kernel and userspace. There's no point in trying to fix them one by one here, because untill it's easy to diff the files with the kernel code these sorts of issues will be easily missed.
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
PATCH 4/14 xfsprogs: fix gpl headers in xfs_symlink
In reply to Ben MyersJust a reminder to make sure this is done, as I think it may be in the 2nd series.
--- include/xfs_symlink.h | 1 + libxfs/xfs_symlink.c | 1 + 2 files changed, 2 insertions(+)
include/xfs_symlink.h +1 -0
@@ -1,5 +1,6 @@/* * Copyright (c) 2012 Red Hat, Inc. All rights reserved. +* XXX fix gpl header */ #ifndef __XFS_SYMLINK_H#define __XFS_SYMLINK_H 1
libxfs/xfs_symlink.c +1 -0
@@ -1,6 +1,7 @@/* * Copyright 2013 Red Hat, Inc. * All rights reserved. +* XXX gpl header needs to be fixed */ #include "xfs.h"
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 5/14 xfsprogs: sync commit f5f3d9b016 completely
In reply to Ben MyersTODO
--- include/xfs_dir2_format.h | 2 +- libxfs/xfs_dir2_data.c | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-)
include/xfs_dir2_format.h +1 -1
@@ -247,7 +247,7 @@ typedef struct xfs_dir2_data_free {*/ typedef struct xfs_dir2_data_hdr {__be32 magic;/* XFS_DIR2_DATA_MAGIC or */ -/* XFS_DIR2_BLOCK_MAGIC */ +/* XFS_DIR2_BLOCK_MAGIC */ xfs_dir2_data_free_t bestfree[XFS_DIR2_DATA_FD_COUNT ]; }xfs_dir2_data_hdr_t ;
libxfs/xfs_dir2_data.c +2 -0
@@ -54,6 +54,7 @@ __xfs_dir2_data_check( p = (char *)xfs_dir3_data_entry_p (hdr);switch (be32_to_cpu (hdr->magic)) { +/* XXX bpm endian switch does not match commit */ case XFS_DIR2_BLOCK_MAGIC :case XFS_DIR3_BLOCK_MAGIC : btp =xfs_dir2_block_tail_p (mp, hdr);
@@ -203,6 +204,7 @@ xfs_dir2_data_reada_verify(struct xfs_dir2_data_hdr *hdr = bp->b_addr;switch (be32_to_cpu (hdr->magic)) { +/* XXX bpm: endian switch does not match kernel commit */ case XFS_DIR2_BLOCK_MAGIC :case XFS_DIR3_BLOCK_MAGIC : bp->b_ops = &xfs_dir3_block_buf_ops;
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 5/14] xfsprogs: sync commit f5f3d9b016 completely
Quoted 25 lines · Ben Myers
TODO
--- include/xfs_dir2_format.h | 2 +- libxfs/xfs_dir2_data.c | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-)Index:
b/include/xfs_dir2_format.h
===================================================================include/xfs_dir2_format.h +1
-1@@ -247,7 +247,7 @@ typedef struct xfs_dir2_data_free {*/ typedef struct xfs_dir2_data_hdr {__be32 magic;/* XFS_DIR2_DATA_MAGIC or */ -/* XFS_DIR2_BLOCK_MAGIC */ +/* XFS_DIR2_BLOCK_MAGIC */ xfs_dir2_data_free_t bestfree[XFS_DIR2_DATA_FD_COUNT ]; }xfs_dir2_data_hdr_t ;Index:
b/libxfs/xfs_dir2_data.c
===================================================================
libxfs/xfs_dir2_data.c __xfs_dir2_data_check( · line 57
Quoted 4 lines
libxfs/xfs_dir2_data.c +1
-0@@ -54,6 +54,7 @@ __xfs_dir2_data_check( p = (char *)xfs_dir3_data_entry_p (hdr);switch (be32_to_cpu (hdr->magic)) { +/* XXX bpm endian switch does not match commit */
in userspace, for some reason, doing it the "kernel way"
(i.e.
switch (hdr->magic) {
case cpu_to_be32(XFS_DIR2_BLOCK_MAGIC):
...)
yields:
xfs_dir2_data.c:57: error: case label does not reduce to an integer constant
-Eric
libxfs/xfs_dir2_data.c xfs_dir2_data_reada_verify( · line 210
Quoted 12 lines
libxfs/xfs_dir2_data.c +1
-0case XFS_DIR2_BLOCK_MAGIC :case XFS_DIR3_BLOCK_MAGIC : btp =xfs_dir2_block_tail_p (mp, hdr);@@ -203,6 +204,7 @@ xfs_dir2_data_reada_verify(struct xfs_dir2_data_hdr *hdr = bp->b_addr;switch (be32_to_cpu (hdr->magic)) { +/* XXX bpm: endian switch does not match kernel commit */ case XFS_DIR2_BLOCK_MAGIC :case XFS_DIR3_BLOCK_MAGIC : bp->b_ops = &xfs_dir3_block_buf_ops;
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
***** SUSPECTED SPAM ***** Re: [PATCH 5/14] xfsprogs: sync commit f5f3d9b016 completely
Quoted 35 lines · Eric Sandeen
Quoted 25 lines · Ben Myers
TODO
--- include/xfs_dir2_format.h | 2 +- libxfs/xfs_dir2_data.c | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-)Index:
b/include/xfs_dir2_format.h
===================================================================include/xfs_dir2_format.h +1
-1@@ -247,7 +247,7 @@ typedef struct xfs_dir2_data_free {*/ typedef struct xfs_dir2_data_hdr {__be32 magic;/* XFS_DIR2_DATA_MAGIC or */ -/* XFS_DIR2_BLOCK_MAGIC */ +/* XFS_DIR2_BLOCK_MAGIC */ xfs_dir2_data_free_t bestfree[XFS_DIR2_DATA_FD_COUNT ]; }xfs_dir2_data_hdr_t ;Index:
b/libxfs/xfs_dir2_data.c
===================================================================libxfs/xfs_dir2_data.c __xfs_dir2_data_check( · line 57
Quoted 4 lines
libxfs/xfs_dir2_data.c +1
-0@@ -54,6 +54,7 @@ __xfs_dir2_data_check( p = (char *)xfs_dir3_data_entry_p (hdr);switch (be32_to_cpu (hdr->magic)) { +/* XXX bpm endian switch does not match commit */ in userspace, for some reason, doing it the "kernel way"
(i.e.
Quoted 8 lines
switch (
hdr->magic) {
casecpu_to_be32(XFS_DIR2_BLOCK_MAGIC):...)
yields:
xfs_dir2_data.c:57: error: case label does not reduce to an integer constant
Right, and as I've already pointed out previously it's fixed in the second series of patches by this:
[PATCH 06/49] libxfs: fix byte swapping on constants
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
PATCH 6/14 xfsprogs: cleanup some whitespace
In reply to Ben MyersThis whitespace was added in patch 10 of the crc-dev series.
The extra ; in xfs_dir3_free_get_buf was taken care of in another patch.
--- libxfs/xfs_dir2_node.c | 3 --- 1 file changed, 3 deletions(-)
libxfs/xfs_dir2_node.c +0 -3
@@ -257,7 +257,6 @@ xfs_dir3_free_get_buf( hdr3->hdr.blkno =cpu_to_be64 (bp->b_bn); hdr3->hdr.owner =cpu_to_be64 (dp->i_ino);uuid_copy (&hdr3->hdr.uuid, &mp->m_sb.sb_uuid); - }else hdr.magic =XFS_DIR2_FREE_MAGIC ;xfs_dir3_free_hdr_to_disk (bp->b_addr, &hdr);
@@ -1101,7 +1100,6 @@ xfs_dir3_data_block_free(__be16 *bests;struct xfs_dir3_icfree_hdr freehdr; -xfs_dir3_free_hdr_from_disk (&freehdr, free); bests =xfs_dir3_free_bests_p (tp->t_mountp, free);
@@ -1159,7 +1157,6 @@ xfs_dir3_data_block_free(*/ } -/* Log the free entry that changed, unless we got rid of it. */ if (logfree)xfs_dir2_free_log_bests (tp, fbp, findex, findex);
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
***** SUSPECTED SPAM ***** Re: [PATCH 6/14] xfsprogs: cleanup some whitespace
Quoted 5 lines · Ben Myers
This whitespace was added in patch 10 of the crc-dev series.
The extra ; in
xfs_dir3_free_get_bufwas taken care of in another patch.Signed-off-by Ben Myers <[email protected]>
Don't bother, it's fixed in the second series of patches.
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
PATCH 7/14 xfsprogs: fix issues with commit 75c8b4343abb
In reply to Ben MyersThese are various issues found in 75c8b4343abb during review.
* clean up a few extra tabs
* xfs_buf_map->xfs_buf_ops in libxfs_readbuf and libxfs_readbuf_map args
* don't call the write verifier twice
* put the multithreaded scan_ags back
--- include/libxfs.h | 2 +- libxfs/rdwr.c | 20 ++------------------ repair/scan.c | 3 +-- 3 files changed, 4 insertions(+), 21 deletions(-)
include/libxfs.h +1 -1
@@ -528,7 +528,7 @@ typedef struct xfs_inode {xfs_mount_t *i_mount;/* fs mount struct ptr */ xfs_ino_t i_ino;/* inode number (agno/agino) */ struct xfs_imap i_imap;/* location for xfs_imap() */ -struct xfs_buftarg i_dev;/* dev for this inode */ +struct xfs_buftarg i_dev;/* dev for this inode */ xfs_ifork_t *i_afp;/* attribute fork pointer */ xfs_ifork_t i_df;/* data fork */ xfs_trans_t *i_transp;/* ptr to owning transaction */
libxfs/rdwr.c +2 -18
@@ -201,9 +201,9 @@ libxfs_log_header(#undef libxfs_putbufxfs_buf_t *libxfs_readbuf (struct xfs_buftarg *,xfs_daddr_t ,int ,int , -const struct xfs_buf_map *); +const struct xfs_buf_ops *);xfs_buf_t *libxfs_readbuf_map (struct xfs_buftarg *,struct xfs_buf_map *, -int ,int ,const struct xfs_buf_map *); +int ,int ,const struct xfs_buf_ops *);int libxfs_writebuf (xfs_buf_t *,int );xfs_buf_t *libxfs_getbuf (struct xfs_buftarg *,xfs_daddr_t ,int );xfs_buf_t *libxfs_getbuf_map (struct xfs_buftarg *,struct xfs_buf_map *,int );
@@ -834,22 +834,6 @@ libxfs_writebufr(xfs_buf_t *bp) } } -/* -* clear any pre-existing error status on the buffer. This can occur if -* the buffer is corrupt on disk and the repair process doesn't clear -* the error before fixing and writing it back. -*/ - bp->b_error =0 ; -if (bp->b_ops) { - bp->b_ops->verify_write (bp); -if (bp->b_error) { -fprintf (stderr, -_ ("%s: write verifer failed on bno 0x%llx/0x%x\n" ), - __func__, (long long )bp->b_bn, bp->b_bcount); -return bp->b_error; - } - } -if (!(bp->b_flags &LIBXFS_B_DISCONTIG )) { error =__write_buf (fd, bp->b_addr, bp->b_bcount,LIBXFS_BBTOOFF64 (bp->b_bn), bp->b_flags);
repair/scan.c +1 -2
@@ -1369,8 +1369,7 @@ scan_ags( }memset (agcnts,0 , mp->m_sb.sb_agcount *sizeof (*agcnts)); -create_work_queue (&wq, mp,1 ); -//create_work_queue(&wq, mp, scan_threads); +create_work_queue (&wq, mp, scan_threads);for (i =0 ; i < mp->m_sb.sb_agcount; i++)queue_work (&wq, scan_ag, i, &agcnts[i]);
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
***** SUSPECTED SPAM ***** Re: [PATCH 7/14] xfsprogs: fix issues with commit 75c8b4343abb
Quoted 7 lines · Ben Myers
These are various issues found in
75c8b4343abbduring review.
* clean up a few extra tabs
*xfs_buf_map->xfs_buf_opsinlibxfs_readbufandlibxfs_readbuf_mapargs
* don't call the write verifier twice
* put the multithreadedscan_agsbackSigned-off-by Ben Myers <[email protected]>
Needs a better subject, say, "clean up libxfs buffer read/write code"?
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
PATCH 8/14 xfsprogs: fix issues with e0607266f23
In reply to Ben MyersA couple of issues found in review.
--- libxfs/xfs_alloc.c | 9 ++------- repair/dinode.c | 9 +++------ 2 files changed, 5 insertions(+), 13 deletions(-)
libxfs/xfs_alloc.c +2 -7
@@ -2173,13 +2173,8 @@ xfs_agf_verify(struct xfs_agf *agf =XFS_BUF_TO_AGF (bp);if (xfs_sb_version_hascrc (&mp->m_sb) && - !uuid_equal (&agf->agf_uuid, &mp->m_sb.sb_uuid)) { -char uu[64 ], uu2[64 ]; -platform_uuid_unparse (&agf->agf_uuid, uu); -platform_uuid_unparse (&mp->m_sb.sb_uuid, uu2); - -return false; - } + !uuid_equal (&agf->agf_uuid, &mp->m_sb.sb_uuid)) +return false; if (!(agf->agf_magicnum == cpu_to_be32(XFS_AGF_MAGIC) && XFS_AGF_GOOD_VERSION(be32_to_cpu(agf->agf_versionnum)) &&
repair/dinode.c +3 -6
@@ -182,12 +182,9 @@ clear_dinode_core(struct xfs_mount *mp,platform_uuid_copy (&dinoc->di_uuid, &mp->m_sb.sb_uuid); } -for (i =0 ; i <16 ; i++) { -if (dinoc->di_pad[i] !=0 ) { -__dirty_no_modify_ret (dirty); -memset (dinoc->di_pad,0 ,16 ); -break ; - } +if (dinoc->di_pad2 !=0 ) { +__dirty_no_modify_ret (dirty); + dinoc->di_pad2 =0 ; }if (be64_to_cpu (dinoc->di_flags2) !=0 ) {
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 8/14] xfsprogs: fix issues with e0607266f23
Quoted 8 lines · Ben Myers
A couple of issues found in review.
Signed-off-by Ben Myers <[email protected]>--- libxfs/xfs_alloc.c | 9 ++------- repair/dinode.c | 9 +++------ 2 files changed, 5 insertions(+), 13 deletions(-)
...
Quoted 7 lines
Index:
b/repair/dinode.c
===================================================================repair/dinode.c +0
-0@@ -182,12 +182,9 @@ clear_dinode_core(struct xfs_mount *mp,platform_uuid_copy (&dinoc->di_uuid, &mp->m_sb.sb_uuid); }
repair/dinode.c clear_dinode_core(struct xfs_mount *mp, · line 187
Quoted 10 lines
repair/dinode.c +3
-6-for (i =0 ; i <16 ; i++) { -if (dinoc->di_pad[i] !=0 ) { -__dirty_no_modify_ret (dirty); -memset (dinoc->di_pad,0 ,16 ); -break ; - } +if (dinoc->di_pad2 !=0 ) { +__dirty_no_modify_ret (dirty); + dinoc->di_pad2 =0 ;
this probably needs to be fixed pronto, it's a memory corruptor right?
w/ a proper commit log,
Thanks,
-Eric
repair/dinode.c clear_dinode_core(struct xfs_mount *mp, · line 190
Quoted 4 lines
repair/dinode.c +0
-0}if (be64_to_cpu (dinoc->di_flags2) !=0 ) {
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 8/14] xfsprogs: fix issues with e0607266f23
Quoted 33 lines · Eric Sandeen
Quoted 8 lines · Ben Myers
A couple of issues found in review.
Signed-off-by Ben Myers <[email protected]>--- libxfs/xfs_alloc.c | 9 ++------- repair/dinode.c | 9 +++------ 2 files changed, 5 insertions(+), 13 deletions(-)...
Quoted 7 lines
Index:
b/repair/dinode.c
===================================================================repair/dinode.c +0
-0@@ -182,12 +182,9 @@ clear_dinode_core(struct xfs_mount *mp,platform_uuid_copy (&dinoc->di_uuid, &mp->m_sb.sb_uuid); }repair/dinode.c clear_dinode_core(struct xfs_mount *mp, · line 187
Quoted 10 lines
repair/dinode.c +3
-6-for (i =0 ; i <16 ; i++) { -if (dinoc->di_pad[i] !=0 ) { -__dirty_no_modify_ret (dirty); -memset (dinoc->di_pad,0 ,16 ); -break ; - } +if (dinoc->di_pad2 !=0 ) { +__dirty_no_modify_ret (dirty); + dinoc->di_pad2 =0 ;this probably needs to be fixed pronto, it's a memory corruptor right?
Quoted 3 lines
w/ a proper commit log,
Reviewed-by Eric Sandeen <[email protected]>
Actually; everywhere else that sets di_pad to 0 does it through a memset (sizeof . . ) - that might be best, on the off chance that di_pad ever changes, rather than setting it to = 0 ?
-Eric
Quoted 5 lines
Thanks,
-EricQuoted 2 lines
repair/dinode.c +0
-0}
Quoted 8 lines
repair/dinode.c clear_dinode_core(struct xfs_mount *mp, · line 190
Quoted 2 lines
repair/dinode.c +0
-0if (be64_to_cpu (dinoc->di_flags2) !=0 ) {_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH v2 8/14 xfsprogs: fix issues with e0607266f23
* remove unused uuid unparse in xfs_agf_verify
* fix an unnecessary loop in clear_dinode_core
---
[v2: address Eric's suggestions]
Eric,
Seems like you are correct, we should get this in pronto.
-Ben
libxfs/xfs_alloc.c | 9 ++------- repair/dinode.c | 10 +++------- 2 files changed, 5 insertions(+), 14 deletions(-)
libxfs/xfs_alloc.c +2 -7
@@ -2173,13 +2173,8 @@ xfs_agf_verify(struct xfs_agf *agf =XFS_BUF_TO_AGF (bp);if (xfs_sb_version_hascrc (&mp->m_sb) && - !uuid_equal (&agf->agf_uuid, &mp->m_sb.sb_uuid)) { -char uu[64 ], uu2[64 ]; -platform_uuid_unparse (&agf->agf_uuid, uu); -platform_uuid_unparse (&mp->m_sb.sb_uuid, uu2); - -return false; - } + !uuid_equal (&agf->agf_uuid, &mp->m_sb.sb_uuid)) +return false; if (!(agf->agf_magicnum == cpu_to_be32(XFS_AGF_MAGIC) && XFS_AGF_GOOD_VERSION(be32_to_cpu(agf->agf_versionnum)) &&
repair/dinode.c +3 -7
@@ -88,7 +88,6 @@ static int clear_dinode_core(struct xfs_mount *mp, xfs_dinode_t *dinoc, xfs_ino_t ino_num) { int dirty = 0; -int i;#define __dirty_no_modify_ret (dirty) \ ({ (dirty) = 1; if (no_modify) return 1; })
@@ -182,12 +181,9 @@ clear_dinode_core(struct xfs_mount *mp,platform_uuid_copy (&dinoc->di_uuid, &mp->m_sb.sb_uuid); } -for (i =0 ; i <16 ; i++) { -if (dinoc->di_pad[i] !=0 ) { -__dirty_no_modify_ret (dirty); -memset (dinoc->di_pad,0 ,16 ); -break ; - } +if (dinoc->di_pad2 !=0 ) { +__dirty_no_modify_ret (dirty); +memset (dinoc->di_pad2,0 ,sizeof (dinoc->di_pad2)); }if (be64_to_cpu (dinoc->di_flags2) !=0 ) {
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [v2 PATCH 8/14] xfsprogs: fix issues with e0607266f23
Quoted 2 lines · Ben Myers
* remove unused uuid unparse in
xfs_agf_verify
* fix an unnecessary loop inclear_dinode_core
These should be 2 commits (they do 2 different things), with properly descriptive summaries & changelogs.
For the 2nd, it's not an unnecessary loop, it's a memory corruptor; that should be noted in the changelog.
TBH I've only reviewed the latter, I need to look at the first.
-Eric
Quoted 62 lines
Signed-off-by Ben Myers <[email protected]>---
[v2: address Eric's suggestions]Eric,
Seems like you are correct, we should get this in pronto.
-Benlibxfs/xfs_alloc.c | 9 ++------- repair/dinode.c | 10 +++------- 2 files changed, 5 insertions(+), 14 deletions(-)Index:
b/libxfs/xfs_alloc.c
===================================================================libxfs/xfs_alloc.c +2
-7@@ -2173,13 +2173,8 @@ xfs_agf_verify(struct xfs_agf *agf =XFS_BUF_TO_AGF (bp);if (xfs_sb_version_hascrc (&mp->m_sb) && - !uuid_equal (&agf->agf_uuid, &mp->m_sb.sb_uuid)) { -char uu[64 ], uu2[64 ]; -platform_uuid_unparse (&agf->agf_uuid, uu); -platform_uuid_unparse (&mp->m_sb.sb_uuid, uu2); - -return false; - } + !uuid_equal (&agf->agf_uuid, &mp->m_sb.sb_uuid)) +return false; if (!(agf->agf_magicnum == cpu_to_be32(XFS_AGF_MAGIC) && XFS_AGF_GOOD_VERSION(be32_to_cpu(agf->agf_versionnum)) &&Index:
b/repair/dinode.c
===================================================================repair/dinode.c +3
-7@@ -88,7 +88,6 @@ static int clear_dinode_core(struct xfs_mount *mp, xfs_dinode_t *dinoc, xfs_ino_t ino_num) { int dirty = 0; -int i;#define __dirty_no_modify_ret (dirty) \ ({ (dirty) = 1; if (no_modify) return 1; })@@ -182,12 +181,9 @@ clear_dinode_core(struct xfs_mount *mp,platform_uuid_copy (&dinoc->di_uuid, &mp->m_sb.sb_uuid); } -for (i =0 ; i <16 ; i++) { -if (dinoc->di_pad[i] !=0 ) { -__dirty_no_modify_ret (dirty); -memset (dinoc->di_pad,0 ,16 ); -break ; - } +if (dinoc->di_pad2 !=0 ) { +__dirty_no_modify_ret (dirty); +memset (dinoc->di_pad2,0 ,sizeof (dinoc->di_pad2)); }if (be64_to_cpu (dinoc->di_flags2) !=0 ) {
Quoted 5 lines
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [v2 PATCH 8/14] xfsprogs: fix issues with e0607266f23
Quoted 4 lines · Eric Sandeen
Quoted 2 lines · Ben Myers
* remove unused uuid unparse in
xfs_agf_verify
* fix an unnecessary loop inclear_dinode_core
Quoted 8 lines
These should be 2 commits (they do 2 different things), with properly descriptive summaries & changelogs.
For the 2nd, it's not an unnecessary loop, it's a memory corruptor; that should be noted in the changelog.
TBH I've only reviewed the latter, I need to look at the first.
Yup the first is fine too, but should be a separate commit.
Thanks,
-Eric
Quoted 3 lines
-Eric
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [v2 PATCH 8/14] xfsprogs: fix issues with e0607266f23
Quoted 8 lines · Eric Sandeen
Quoted 7 lines · Eric Sandeen
Quoted 2 lines · Ben Myers
* remove unused uuid unparse in
xfs_agf_verify
* fix an unnecessary loop inclear_dinode_coreThese should be 2 commits (they do 2 different things), with properly descriptive summaries & changelogs.
Quoted 7 lines
Quoted 5 lines
For the 2nd, it's not an unnecessary loop, it's a memory corruptor; that should be noted in the changelog.
TBH I've only reviewed the latter, I need to look at the first.
Yup the first is fine too, but should be a separate commit.
Sounds good, thanks Eric.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 1/1 xfsprogs: issues with a24374f41c9
In reply to Ben MyersThis patch corresponds with kernel commit 517c22207b04. There were a couple bits that didn't match when it was copied to libxfs.
--- libxfs/xfs_attr.c | 2 +- libxfs/xfs_attr_leaf.c | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-)
libxfs/xfs_attr.c +1 -1
@@ -861,7 +861,7 @@ xfs_attr_leaf_removename(xfs_da_args_t * error =xfs_attr3_leaf_lookup_int (bp, args);if (error ==ENOATTR ) {xfs_trans_brelse (args->trans, bp); -return (error); +return error; }xfs_attr3_leaf_remove (bp, args);
libxfs/xfs_attr_leaf.c +1 -2
@@ -1111,7 +1111,6 @@ xfs_attr3_leaf_add_work(struct xfs_attr_leaf_entry *entry;struct xfs_attr_leaf_name_local *name_loc;struct xfs_attr_leaf_name_remote *name_rmt; -struct xfs_attr_leaf_map *map;struct xfs_mount *mp;int tmp;int i;
@@ -1210,7 +1209,7 @@ xfs_attr3_leaf_add_work( tmp = (ichdr->count -1 ) *sizeof (xfs_attr_leaf_entry_t) +xfs_attr3_leaf_hdr_size (leaf); -for (i =0 ; i <XFS_ATTR_LEAF_MAPSIZE ; map++, i++) { +for (i =0 ; i <XFS_ATTR_LEAF_MAPSIZE ; i++) {if (ichdr->freemap[i].base == tmp) { ichdr->freemap[i].base +=sizeof (xfs_attr_leaf_entry_t); ichdr->freemap[i].size -=sizeof (xfs_attr_leaf_entry_t);
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 0/14] xfsprogs: various issues from review
In reply to Ben MyersQuoted 26 lines · Ben Myers
Quoted 21 lines · Ben Myers
Quoted 20 lines · Dave Chinner
Hi folks,
This is the latest update of the series of patches tht introduces CRC support into xfsprogs. Of note, for CRC enabled filesystems;
- write support for xfs-db is disabled
- obfuscation for metadump is disabled
-xfs_checkdoes nothing ("always succeed") so that xfstests can run without needing this
- all structures shoul dbe supported for printing inxfs_db
-xfs_repairshould be able to fully validate the structure of a CRC enabled filesystem.
-xfs_repairstill ignores CRC validation errors when reading metadata
- mkfs.xfs enforces limitations on the format of CRC enabled filesystems (inode size, attr format, projid32bit, etc).
- whenever a v5 superblock is parsed on read by any utility, it outputs a wanring about it being an experimental format.Quoted 3 lines
Quoted 1 line
Bug reports, patches, comments, reviews, etc all welcome.
Pulled in 1-48 of the first series and 1-12 of the second.
Quoted 7 lines
Here is a patch series that addresses some of my outstanding concerns from review. Some may be already fixed in the 2nd series, I'm not sure. Eric also mentioned that he put the updated branch through coverity and found some defects. There may be some overlap there too.
Some of these are just reminders for myself to make sure certain items are addressed eventually. Sorry for the noise.
I'll stop at 9. The rest of them are notes to myself:
* xfs_db write support needs to be done
* xfs_metadump obfuscation needs to be done
* xfs_mdrestore needs to work
* xfs_check needs work
-Ben
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
***** SUSPECTED SPAM ***** Re: [PATCH 0/14] xfsprogs: various issues from review
Quoted 32 lines · Ben Myers
Quoted 31 lines · Ben Myers
Quoted 21 lines · Ben Myers
Quoted 20 lines · Dave Chinner
Hi folks,
This is the latest update of the series of patches tht introduces CRC support into xfsprogs. Of note, for CRC enabled filesystems;
- write support for xfs-db is disabled
- obfuscation for metadump is disabled
-xfs_checkdoes nothing ("always succeed") so that xfstests can run without needing this
- all structures shoul dbe supported for printing inxfs_db
-xfs_repairshould be able to fully validate the structure of a CRC enabled filesystem.
-xfs_repairstill ignores CRC validation errors when reading metadata
- mkfs.xfs enforces limitations on the format of CRC enabled filesystems (inode size, attr format, projid32bit, etc).
- whenever a v5 superblock is parsed on read by any utility, it outputs a wanring about it being an experimental format.Quoted 3 lines
Quoted 1 line
Bug reports, patches, comments, reviews, etc all welcome.
Pulled in 1-48 of the first series and 1-12 of the second.
Here is a patch series that addresses some of my outstanding concerns from review. Some may be already fixed in the 2nd series, I'm not sure. Eric also mentioned that he put the updated branch through coverity and found some defects. There may be some overlap there too.
Quoted 6 lines
Quoted 2 lines
Some of these are just reminders for myself to make sure certain items are addressed eventually. Sorry for the noise.
I'll stop at 9. The rest of them are notes to myself:
*
xfs_dbwrite support needs to be done
Dependent on being able to write crcs. needs xfs_db to be converted to libxfs based IO.
Quoted 1 line
*
xfs_metadumpobfuscation needs to be done
dependent on the same thing as xfs_db write support.
Quoted 1 line
*
xfs_mdrestoreneeds to work
Shoul dwork if xfs_metadump works properly.
Quoted 1 line
*
xfs_checkneeds work
Probably not. It's deprecated.
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs