xfs: miscellaneous improvements for 4.10
cover xfs: miscellaneous improvements for 4.10
Hi folks,
Some things that need review, hoepfully in time for the 4.10 cycle. I've been running all these in my test kernels for some time now, and they all seem stable and perform as expected. The btree stats patch substantially reduces the icache footprint of the core btree code. The rhashtable patch (many thanks to Lucas for writing the original patch!) gets rid of most of the _xfs_buf_find() lookup overhead. The CRC update optimisation patch (thanks to the diagnosis, analysis and prototyping work done by Nick Piggin) removes CRCs almost entirely from my kernel profiles on metadata intensive workloads.
Comments, flames and thoughts welcome.
-Dave.
PATCH 1/3 xfs: make xfs btree stats less huge
From: Dave Chinner <[email protected]>
Embedding a switch statement in every btree stats inc/add adds a lot of code overhead to the core btree infrastructure paths. Stats are supposed to be small and lightweight, but the btree stats have become big and bloated as we've added more btrees. It needs fixing because the reflink code will just add more overhead again.
Convert the v2 btree stats to arrays instead of independent variables, and instead use the type to index the specific btree array via an enum. This allows us to use array based indexing to update the stats, rather than having to derefence variables specific to the btree type.
If we then wrap the xfsstats structure in a union and place uint32_t array beside it, and calculate the correct btree stats array base array index when creating a btree cursor, we can easily access entries in the stats structure without having to switch names based on the btree type.
We then replace with the switch statement with a simple set of stats wrapper macros, resulting in a significant simplification of the btree stats code, and:
text data bss dec hex filename
48905 144 8 49057 bfa1 fs/xfs/libxfs/xfs_btree.o.old 36793 144 8 36945 9051 fs/xfs/libxfs/xfs_btree.o
it reduces the core btree infrastructure code size by close to 25%!
--- fs/xfs/libxfs/xfs_alloc_btree.c | 4 + fs/xfs/libxfs/xfs_bmap_btree.c | 1 + fs/xfs/libxfs/xfs_btree.h | 43 +-------- fs/xfs/libxfs/xfs_ialloc_btree.c | 2 + fs/xfs/libxfs/xfs_refcount_btree.c | 1 + fs/xfs/libxfs/xfs_rmap_btree.c | 1 + fs/xfs/xfs_stats.c | 10 +- fs/xfs/xfs_stats.h | 192 ++++++++++++++----------------------- 8 files changed, 91 insertions(+), 163 deletions(-)
fs/xfs/libxfs/xfs_alloc_btree.c +4 -0
@@ -428,6 +428,10 @@ xfs_allocbt_init_cursor( cur->bc_btnum = btnum; cur->bc_blocklog = mp->m_sb.sb_blocklog; cur->bc_ops = &xfs_allocbt_ops; +if (btnum ==XFS_BTNUM_BNO ) + cur->bc_statoff =XFS_STATS_CALC_INDEX (xs_abtb_2); +else + cur->bc_statoff =XFS_STATS_CALC_INDEX (xs_abtc_2);if (btnum ==XFS_BTNUM_CNT ) { cur->bc_nlevels =be32_to_cpu (agf->agf_levels[XFS_BTNUM_CNT ]);
fs/xfs/libxfs/xfs_bmap_btree.c +1 -0
@@ -803,6 +803,7 @@ xfs_bmbt_init_cursor( cur->bc_nlevels =be16_to_cpu (ifp->if_broot->bb_level) +1 ; cur->bc_btnum =XFS_BTNUM_BMAP ; cur->bc_blocklog = mp->m_sb.sb_blocklog; + cur->bc_statoff =XFS_STATS_CALC_INDEX (xs_bmbt_2); cur->bc_ops = &xfs_bmbt_ops; cur->bc_flags =XFS_BTREE_LONG_PTRS |XFS_BTREE_ROOT_IN_INODE ;
fs/xfs/libxfs/xfs_btree.h +4 -39
56 lines not on this page · raw message
fs/xfs/libxfs/xfs_ialloc_btree.c +2 -0
@@ -365,9 +365,11 @@ xfs_inobt_init_cursor(if (btnum ==XFS_BTNUM_INO ) { cur->bc_nlevels =be32_to_cpu (agi->agi_level); cur->bc_ops = &xfs_inobt_ops; + cur->bc_statoff =XFS_STATS_CALC_INDEX (xs_ibt_2); }else { cur->bc_nlevels =be32_to_cpu (agi->agi_free_level); cur->bc_ops = &xfs_finobt_ops; + cur->bc_statoff =XFS_STATS_CALC_INDEX (xs_fibt_2); } cur->bc_blocklog = mp->m_sb.sb_blocklog;
fs/xfs/libxfs/xfs_refcount_btree.c +1 -0
@@ -354,6 +354,7 @@ xfs_refcountbt_init_cursor( cur->bc_btnum =XFS_BTNUM_REFC ; cur->bc_blocklog = mp->m_sb.sb_blocklog; cur->bc_ops = &xfs_refcountbt_ops; + cur->bc_statoff =XFS_STATS_CALC_INDEX (xs_refcbt_2); cur->bc_nlevels =be32_to_cpu (agf->agf_refcount_level);
fs/xfs/libxfs/xfs_rmap_btree.c +1 -0
@@ -484,6 +484,7 @@ xfs_rmapbt_init_cursor( cur->bc_blocklog = mp->m_sb.sb_blocklog; cur->bc_ops = &xfs_rmapbt_ops; cur->bc_nlevels =be32_to_cpu (agf->agf_levels[XFS_BTNUM_RMAP ]); + cur->bc_statoff =XFS_STATS_CALC_INDEX (xs_rmap_2); cur->bc_private.a.agbp = agbp; cur->bc_private.a.agno = agno;
fs/xfs/xfs_stats.c +5 -5
@@ -80,9 +80,9 @@ int xfs_stats_format(struct xfsstats __percpu *stats, char *buf) }/* extra precision counters */ for_each_possible_cpu (i) { - xs_xstrat_bytes +=per_cpu_ptr (stats, i)->xs_xstrat_bytes; - xs_write_bytes +=per_cpu_ptr (stats, i)->xs_write_bytes; - xs_read_bytes +=per_cpu_ptr (stats, i)->xs_read_bytes; + xs_xstrat_bytes +=per_cpu_ptr (stats, i)->s.xs_xstrat_bytes; + xs_write_bytes +=per_cpu_ptr (stats, i)->s.xs_write_bytes; + xs_read_bytes +=per_cpu_ptr (stats, i)->s.xs_read_bytes; } len += snprintf(buf + len, PATH_MAX-len, "xpc %Lu %Lu %Lu\n",
@@ -106,9 +106,9 @@ void xfs_stats_clearall(struct xfsstats __percpu *stats)for_each_possible_cpu (c) {preempt_disable ();/* save vn_active, it's a universal truth! */ - vn_active =per_cpu_ptr (stats, c)->vn_active; + vn_active =per_cpu_ptr (stats, c)->s.vn_active;memset (per_cpu_ptr (stats, c),0 ,sizeof (*stats)); -per_cpu_ptr (stats, c)->vn_active = vn_active; +per_cpu_ptr (stats, c)->s.vn_active = vn_active;preempt_enable (); } }
fs/xfs/xfs_stats.h +73 -119
227 lines not on this page · raw message
Signature · 1 line
2.10.2
Re: [PATCH 1/3] xfs: make xfs btree stats less huge
fs/xfs/xfs_stats.h line 25
Quoted 9 lines
fs/xfs/xfs_stats.h +2
-0@@ -21,10 +21,30 @@#include <linux/percpu.h> +enum { + __XBTS_lookup =0 ,
Add a comment on top of the enum to explain these are the offsets into each btree type?
fs/xfs/xfs_stats.h struct xfsstats { · line 181
Quoted 2 lines
fs/xfs/xfs_stats.h +2
-0+#define XFS_STATS_CALC_INDEX (member) \ + (offsetof(struct __xfsstats, member) / (int)sizeof(__uint32_t))
what's that int cast for?
Except for these nitpicks this looks great:
Re: [PATCH 1/3] xfs: make xfs btree stats less huge
Quoted 5 lines · Christoph Hellwig
Quoted 5 lines
fs/xfs/xfs_stats.h +0
-0@@ -21,10 +21,30 @@
Quoted 7 lines
fs/xfs/xfs_stats.h line 25
Quoted 4 lines
fs/xfs/xfs_stats.h +2
-0#include <linux/percpu.h> +enum { + __XBTS_lookup =0 ,Add a comment on top of the enum to explain these are the offsets into each btree type?
Will do.
Quoted 4 lines
fs/xfs/xfs_stats.h struct xfsstats { · line 181
Quoted 2 lines
fs/xfs/xfs_stats.h +2
-0+#define XFS_STATS_CALC_INDEX (member) \ + (offsetof(struct __xfsstats, member) / (int)sizeof(__uint32_t))what's that int cast for?
Habit - prevents 64 bit division on 32 bit systems. But given this is evaluated at compile time, that doesn't matter. Will remove.
Quoted 1 line
Reviewed-by Christoph Hellwig <[email protected]>
Thanks!
Signature · 2 lines
Dave Chinner [email protected]
Re: [PATCH 1/3] xfs: make xfs btree stats less huge
Quoted 6 lines · Dave Chinner
Quoted 3 lines
fs/xfs/xfs_stats.h struct xfsstats { · line 181
Quoted 1 line
fs/xfs/xfs_stats.h +1
-0+ (offsetof(struct __xfsstats , member) / (int )sizeof (__uint32_t))what's that int cast for?
Habit - prevents 64 bit division on 32 bit systems. But given this is evaluated at compile time, that doesn't matter. Will remove.
sizeof returns a size_t, which at least on Linux will always be equivalent to an unsigned long, i.e. 32-bit on a 32-bit system, 64-bit on a 64-bit system.
PATCH 2/3 xfs: use rhashtable to track buffer cache
In reply to Dave ChinnerFrom: Lucas Stach <[email protected]>
On filesystems with a lot of metadata and in metadata intensive workloads xfs_buf_find() is showing up at the top of the CPU cycles trace. Most of the CPU time is spent on CPU cache misses while traversing the rbtree.
As the buffer cache does not need any kind of ordering, but fast lookups a hashtable is the natural data structure to use. The rhashtable infrastructure provides a self-scaling hashtable implementation and allows lookups to proceed while the table is going through a resize operation.
This reduces the CPU-time spent for the lookups to 1/3 even for small filesystems with a relatively small number of cached buffers, with possibly much larger gains on higher loaded filesystems.
[dchinner: reduce minimum hash size to an acceptable size for large
filesystems with many AGs with no active use.]
[dchinner: remove stale rbtree asserts.]
[dchinner: use xfs_buf_map for compare function argument.] [dchinner: make functions static.]
[dchinner: remove redundant comments.]
--- fs/xfs/xfs_buf.c | 121 ++++++++++++++++++++++++++++++++--------------------- fs/xfs/xfs_buf.h | 2 +- fs/xfs/xfs_linux.h | 1 + fs/xfs/xfs_mount.c | 7 +++- fs/xfs/xfs_mount.h | 7 +++- 5 files changed, 86 insertions(+), 52 deletions(-)
fs/xfs/xfs_buf.c +74 -47
@@ -219,7 +219,6 @@ _xfs_buf_alloc(init_completion (&bp->b_iowait);INIT_LIST_HEAD (&bp->b_lru);INIT_LIST_HEAD (&bp->b_list); -RB_CLEAR_NODE (&bp->b_rbnode);sema_init (&bp->b_sema,0 );/* held, no waiters */ spin_lock_init (&bp->b_lock);XB_SET_OWNER (bp);
@@ -473,6 +472,63 @@ _xfs_buf_map_pages(/* * Finding and Reading Buffers */ +static int +_xfs_buf_cmp ( +struct rhashtable_compare_arg *arg, +const void *obj) +{ +const struct xfs_buf_map *map = arg->key; +const struct xfs_buf *bp = obj; + +/* +* The key hashing in the lookup path depends on the key being the +* first element of the compare_arg, make sure to assert this. +*/ +BUILD_BUG_ON (offsetof(struct xfs_buf_map , bm_bn) !=0 ); + +if (bp->b_bn == map->bm_bn) { +if (unlikely (bp->b_length != map->bm_len)) { +/* +* found a block number match. If the range doesn't +* match, the only way this is allowed is if the buffer +* in the cache is stale and the transaction that made +* it stale has not yet committed. i.e. we are +* reallocating a busy extent. Skip this buffer and +* continue searching for an exact match. +*/ +ASSERT (bp->b_flags &XBF_STALE ); +return 1 ; + } + +return 0 ; + } + +return 1 ; +} + +static const struct rhashtable_params xfs_buf_hash_params = { + .min_size =32 ,/* empty/unused AGs have minimal footprint */ + .nelem_hint =16 , + .key_len =sizeof (xfs_daddr_t), + .key_offset = offsetof(struct xfs_buf , b_bn), + .head_offset = offsetof(struct xfs_buf , b_rhash_head), + .automatic_shrinking = true, + .obj_cmpfn = _xfs_buf_cmp, +}; + +int xfs_buf_hash_init ( +struct xfs_perag *pag) +{ +spin_lock_init (&pag->pag_buf_lock); +return rhashtable_init (&pag->pag_buf_hash, &xfs_buf_hash_params); +} + +void +xfs_buf_hash_destroy ( +struct xfs_perag *pag) +{ +rhashtable_destroy (&pag->pag_buf_hash); +}/* * Look up, and creates if absent, a lockable buffer for
@@ -488,27 +544,24 @@ _xfs_buf_find( xfs_buf_t *new_bp) {struct xfs_perag *pag; -struct rb_node **rbp; -struct rb_node *parent;xfs_buf_t *bp; -xfs_daddr_t blkno = map[0 ].bm_bn; +struct xfs_buf_map cmap = { .bm_bn = map[0 ].bm_bn };xfs_daddr_t eofs; -int numblks =0 ;int i;for (i =0 ; i < nmaps; i++) - numblks += map[i].bm_len; + cmap.bm_len += map[i].bm_len;/* Check for IOs smaller than the sector size / not sector aligned */ -ASSERT (!(BBTOB (numblks) < btp->bt_meta_sectorsize)); -ASSERT (!(BBTOB (blkno) & (xfs_off_t )btp->bt_meta_sectormask)); +ASSERT (!(BBTOB (cmap.bm_len) < btp->bt_meta_sectorsize)); +ASSERT (!(BBTOB (cmap.bm_bn) & (xfs_off_t )btp->bt_meta_sectormask));/* * Corrupted block numbers can get through to here, unfortunately, so we * have to check that the buffer falls within the filesystem bounds. */ eofs =XFS_FSB_TO_BB (btp->bt_mount, btp->bt_mount->m_sb.sb_dblocks); -if (blkno <0 || blkno >= eofs) { +if (cmap.bm_bn <0 || cmap.bm_bn >= eofs) {/* * XXX (dgc): we should really be returning -EFSCORRUPTED here, * but none of the higher level infrastructure supports
@@ -516,53 +569,29 @@ _xfs_buf_find(*/ xfs_alert (btp->bt_mount,"%s: Block out of range: block 0x%llx, EOFS 0x%llx " , - __func__, blkno, eofs); + __func__, cmap.bm_bn, eofs);WARN_ON (1 );return NULL ; } -/* get tree root */ pag =xfs_perag_get (btp->bt_mount, -xfs_daddr_to_agno (btp->bt_mount, blkno)); +xfs_daddr_to_agno (btp->bt_mount, cmap.bm_bn)); -/* walk tree */ spin_lock (&pag->pag_buf_lock); - rbp = &pag->pag_buf_tree.rb_node; - parent =NULL ; - bp =NULL ; -while (*rbp) { - parent = *rbp; - bp = rb_entry(parent, struct xfs_buf, b_rbnode); - -if (blkno < bp->b_bn) - rbp = &(*rbp)->rb_left; -else if (blkno > bp->b_bn) - rbp = &(*rbp)->rb_right; -else { -/* -* found a block number match. If the range doesn't -* match, the only way this is allowed is if the buffer -* in the cache is stale and the transaction that made -* it stale has not yet committed. i.e. we are -* reallocating a busy extent. Skip this buffer and -* continue searching to the right for an exact match. -*/ -if (bp->b_length != numblks) { -ASSERT (bp->b_flags &XBF_STALE ); - rbp = &(*rbp)->rb_right; -continue ; - } -atomic_inc (&bp->b_hold); - goto found; - } + bp =rhashtable_lookup_fast (&pag->pag_buf_hash, &cmap, + xfs_buf_hash_params); +if (bp) { +atomic_inc (&bp->b_hold); + goto found; }/* No match found */ if (new_bp) { -rb_link_node (&new_bp->b_rbnode, parent, rbp); -rb_insert_color (&new_bp->b_rbnode, &pag->pag_buf_tree);/* the buffer keeps the perag reference until it is freed */ new_bp->b_pag = pag; +rhashtable_insert_fast (&pag->pag_buf_hash, + &new_bp->b_rhash_head, + xfs_buf_hash_params);spin_unlock (&pag->pag_buf_lock); }else {XFS_STATS_INC (btp->bt_mount, xb_miss_locked);
@@ -930,7 +959,6 @@ xfs_buf_rele(if (!pag) {ASSERT (list_empty (&bp->b_lru)); -ASSERT (RB_EMPTY_NODE (&bp->b_rbnode));if (atomic_dec_and_test (&bp->b_hold)) {xfs_buf_ioacct_dec (bp);xfs_buf_free (bp);
@@ -938,8 +966,6 @@ xfs_buf_rele(return ; } -ASSERT (!RB_EMPTY_NODE (&bp->b_rbnode)); -ASSERT (atomic_read (&bp->b_hold) >0 ); release =atomic_dec_and_lock (&bp->b_hold, &pag->pag_buf_lock);
@@ -983,7 +1009,8 @@ xfs_buf_rele( }ASSERT (!(bp->b_flags & _XBF_DELWRI_Q)); -rb_erase (&bp->b_rbnode, &pag->pag_buf_tree); +rhashtable_remove_fast (&pag->pag_buf_hash, &bp->b_rhash_head, + xfs_buf_hash_params);spin_unlock (&pag->pag_buf_lock);xfs_perag_put (pag); freebuf = true;
fs/xfs/xfs_buf.h +1 -1
@@ -151,7 +151,7 @@ typedef struct xfs_buf {* which is the only bit that is touched if we hit the semaphore * fast-path on locking. */ -struct rb_node b_rbnode;/* rbtree node */ +struct rhash_head b_rhash_head;/* pag buffer hash node */ xfs_daddr_t b_bn;/* block number of buffer */ int b_length;/* size of buffer in BBs */ atomic_t b_hold;/* reference count */
fs/xfs/xfs_linux.h +1 -0
@@ -78,6 +78,7 @@ typedef __u32 xfs_nlink_t;#include <linux/freezer.h> #include <linux/list_sort.h> #include <linux/ratelimit.h> +#include <linux/rhashtable.h> #include <asm/page.h> #include <asm/div64.h>
fs/xfs/xfs_mount.c +5 -2
@@ -157,6 +157,7 @@ xfs_free_perag(spin_unlock (&mp->m_perag_lock);ASSERT (pag);ASSERT (atomic_read (&pag->pag_ref) ==0 ); +xfs_buf_hash_destroy (pag);call_rcu (&pag->rcu_head, __xfs_free_perag); } }
@@ -212,8 +213,8 @@ xfs_initialize_perag(spin_lock_init (&pag->pag_ici_lock);mutex_init (&pag->pag_ici_reclaim_lock);INIT_RADIX_TREE (&pag->pag_ici_root,GFP_ATOMIC ); -spin_lock_init (&pag->pag_buf_lock); - pag->pag_buf_tree =RB_ROOT ; +if (xfs_buf_hash_init (pag)) + goto out_unwind;if (radix_tree_preload (GFP_NOFS )) goto out_unwind;
@@ -239,9 +240,11 @@ xfs_initialize_perag(return 0 ; out_unwind: +xfs_buf_hash_destroy (pag);kmem_free (pag);for (; index > first_initialised; index--) { pag =radix_tree_delete (&mp->m_perag_tree, index); +xfs_buf_hash_destroy (pag);kmem_free (pag); }return error;
fs/xfs/xfs_mount.h +5 -2
@@ -393,8 +393,8 @@ typedef struct xfs_perag {unsigned long pag_ici_reclaim_cursor;/* reclaim restart point */ /* buffer cache index */ -spinlock_t pag_buf_lock;/* lock for pag_buf_tree */ -struct rb_root pag_buf_tree;/* ordered tree of active buffers */ +spinlock_t pag_buf_lock;/* lock for pag_buf_hash */ +struct rhashtable pag_buf_hash;/* for rcu-safe freeing */ struct rcu_head rcu_head;
@@ -424,6 +424,9 @@ xfs_perag_resv( } } +int xfs_buf_hash_init (xfs_perag_t *pag); +void xfs_buf_hash_destroy (xfs_perag_t *pag); +extern void xfs_uuid_table_free (void );extern int xfs_log_sbcount (xfs_mount_t *);extern __uint64_t xfs_default_resblks (xfs_mount_t *mp);
Signature · 1 line
2.10.2
Re: [PATCH 2/3] xfs: use rhashtable to track buffer cache
fs/xfs/xfs_buf.c _xfs_buf_map_pages( · line 481
Quoted 7 lines
fs/xfs/xfs_buf.c +7
-0+static int +_xfs_buf_cmp ( +struct rhashtable_compare_arg *arg, +const void *obj) +{ +const struct xfs_buf_map *map = arg->key; +const struct xfs_buf *bp = obj;
How about aligning the parameter / variable names here like we normally do in XFS?
fs/xfs/xfs_buf.c _xfs_buf_map_pages( · line 516
Quoted 8 lines
fs/xfs/xfs_buf.c +8
-0+static const struct rhashtable_params xfs_buf_hash_params = { + .min_size =32 ,/* empty/unused AGs have minimal footprint */ + .nelem_hint =16 , + .key_len =sizeof (xfs_daddr_t), + .key_offset = offsetof(struct xfs_buf , b_bn), + .head_offset = offsetof(struct xfs_buf , b_rhash_head), + .automatic_shrinking = true, + .obj_cmpfn = _xfs_buf_cmp,
Some tab alignments before the equal signs here?
Also please name the compare function so that it fits the field name, as that makes grepping so much easier..
.obj_cmpfn = xfs_buf_obj_cmpfn,
Otherwise this looks fine to me and seems to survive testing so far:
Re: [PATCH 2/3] xfs: use rhashtable to track buffer cache
Quoted 10 lines · Christoph Hellwig
fs/xfs/xfs_buf.c _xfs_buf_map_pages( · line 481
Quoted 7 lines
fs/xfs/xfs_buf.c +7
-0+static int +_xfs_buf_cmp ( +struct rhashtable_compare_arg *arg, +const void *obj) +{ +const struct xfs_buf_map *map = arg->key; +const struct xfs_buf *bp = obj;How about aligning the parameter / variable names here like we normally do in XFS?
Sorry, forgot to do that when I updated the patch.
Quoted 11 lines
fs/xfs/xfs_buf.c _xfs_buf_map_pages( · line 516
Quoted 8 lines
fs/xfs/xfs_buf.c +8
-0+static const struct rhashtable_params xfs_buf_hash_params = { + .min_size =32 ,/* empty/unused AGs have minimal footprint */ + .nelem_hint =16 , + .key_len =sizeof (xfs_daddr_t), + .key_offset = offsetof(struct xfs_buf , b_bn), + .head_offset = offsetof(struct xfs_buf , b_rhash_head), + .automatic_shrinking = true, + .obj_cmpfn = _xfs_buf_cmp,Some tab alignments before the equal signs here?
Quoted 4 lines
Also please name the compare function so that it fits the field name, as that makes grepping so much easier..
.obj_cmpfn = xfs_buf_obj_cmpfn,
Will do.
Quoted 3 lines
Otherwise this looks fine to me and seems to survive testing so far:
Reviewed-by Christoph Hellwig <[email protected]>
Thanks.
-Dave.
Signature · 2 lines
Dave Chinner [email protected]
PATCH 3/3 xfs: optimise CRC updates
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
Nick Piggin reported that the CRC overhead in an fsync heavy workload was higher than expected on a Power8 machine. Part of this was to do with the fact that the power8 CRC implementation is not efficient for CRC lengths of less than 512 bytes, and so the way we split the CRCs over the CRC field means a lot of the CRCs are reduced to being less than than optimal size.
TO otpimise this, change the CRC update mechanism to zero the CRC field first, and then compute the CRC in one pass over the buffer and write the result back into the buffer. We can do this safely because anything writing a CRC has exclusive access to the buffer the CRC is being calculated over.
We leave the CRC verify code the same - it still splits the CRC calculation - because we do not want read-only operations modifying the underlying buffer. This is because read-only operations may not have an exclusive access to the buffer guaranteed, and so temporary modifications could leak out to to other processes accessing the buffer concurrently.
--- fs/xfs/libxfs/xfs_cksum.h | 26 ++++++++++++++++++++++---- fs/xfs/libxfs/xfs_inode_buf.c | 2 +- fs/xfs/xfs_log.c | 2 +- fs/xfs/xfs_log_recover.c | 12 +++++++----- 4 files changed, 31 insertions(+), 11 deletions(-)
fs/xfs/libxfs/xfs_cksum.h +22 -4
@@ -6,10 +6,11 @@/* * Calculate the intermediate checksum for a buffer that has the CRC field * inside it. The offset of the 32bit crc fields is passed as the -* cksum_offset parameter. +* cksum_offset parameter. We do not modify the buffer during verification, +* hence we have to split the CRC calculation across the cksum_offset. */ static inline __uint32_t -xfs_start_cksum (char *buffer,size_t length,unsigned long cksum_offset) +xfs_start_cksum_safe (char *buffer,size_t length,unsigned long cksum_offset) {__uint32_t zero =0 ;__uint32_t crc;
@@ -26,6 +27,20 @@ xfs_start_cksum(char *buffer, size_t length, unsigned long cksum_offset) }/* +* Fast CRC method where the buffer is modified. Callers must have exclusive +* access to the buffer while the calculation takes place. +*/ +static inline __uint32_t +xfs_start_cksum_update (char *buffer,size_t length,unsigned long cksum_offset) +{ +/* zero the CRC field */ + *(__le32 *)(buffer + cksum_offset) =0 ; + +/* single pass CRC calculation for the entire buffer */ +return crc32c (XFS_CRC_SEED , buffer, length); +} + +/* * Convert the intermediate checksum to the final ondisk format. * * The CRC32c calculation uses LE format even on BE machines, but returns the
@@ -40,11 +55,14 @@ xfs_end_cksum(__uint32_t crc)/* * Helper to generate the checksum for a buffer. +* +* This modifies the buffer temporarily - callers must have exclusive +* access to the buffer while the calculation takes place. */ static inline void xfs_update_cksum (char *buffer,size_t length,unsigned long cksum_offset) { -__uint32_t crc =xfs_start_cksum (buffer, length, cksum_offset); +__uint32_t crc =xfs_start_cksum_update (buffer, length, cksum_offset); *(__le32 *)(buffer + cksum_offset) =xfs_end_cksum (crc); }
@@ -55,7 +73,7 @@ xfs_update_cksum(char *buffer, size_t length, unsigned long cksum_offset)static inline int xfs_verify_cksum (char *buffer,size_t length,unsigned long cksum_offset) { -__uint32_t crc =xfs_start_cksum (buffer, length, cksum_offset); +__uint32_t crc =xfs_start_cksum_safe (buffer, length, cksum_offset);return *(__le32 *)(buffer + cksum_offset) ==xfs_end_cksum (crc); }
fs/xfs/libxfs/xfs_inode_buf.c +1 -1
@@ -436,7 +436,7 @@ xfs_dinode_calc_crc(return ;ASSERT (xfs_sb_version_hascrc (&mp->m_sb)); - crc =xfs_start_cksum ((char *)dip, mp->m_sb.sb_inodesize, + crc =xfs_start_cksum_update ((char *)dip, mp->m_sb.sb_inodesize,XFS_DINODE_CRC_OFF ); dip->di_crc =xfs_end_cksum (crc); }
fs/xfs/xfs_log.c +1 -1
@@ -1668,7 +1668,7 @@ xlog_cksum(__uint32_t crc;/* first generate the crc for the record header ... */ - crc =xfs_start_cksum ((char *)rhead, + crc =xfs_start_cksum_update ((char *)rhead,sizeof (struct xlog_rec_header ), offsetof(struct xlog_rec_header , h_crc));
fs/xfs/xfs_log_recover.c +7 -5
@@ -5113,19 +5113,21 @@ xlog_recover_process( struct list_head *buffer_list) {int error; +__le32 old_crc = rhead->h_crc;__le32 crc; + crc =xlog_cksum (log, rhead, dp,be32_to_cpu (rhead->h_len));/* * Nothing else to do if this is a CRC verification pass. Just return * if this a record with a non-zero crc. Unfortunately, mkfs always -* sets h_crc to 0 so we must consider this valid even on v5 supers. +* sets old_crc to 0 so we must consider this valid even on v5 supers. * Otherwise, return EFSBADCRC on failure so the callers up the stack * know precisely what failed. */ if (pass ==XLOG_RECOVER_CRCPASS ) { -if (rhead->h_crc && crc != rhead->h_crc) +if (old_crc && crc != old_crc)return -EFSBADCRC ;return 0 ; }
@@ -5136,11 +5138,11 @@ xlog_recover_process(* zero CRC check prevents warnings from being emitted when upgrading * the kernel from one that does not add CRCs by default. */ -if (crc != rhead->h_crc) { -if (rhead->h_crc ||xfs_sb_version_hascrc (&log->l_mp->m_sb)) { +if (crc != old_crc) { +if (old_crc ||xfs_sb_version_hascrc (&log->l_mp->m_sb)) {xfs_alert (log->l_mp,"log record CRC mismatch: found 0x%x, expected 0x%x." , -le32_to_cpu (rhead->h_crc), +le32_to_cpu (old_crc),le32_to_cpu (crc));xfs_hex_dump (dp,32 ); }
Signature · 1 line
2.10.2
Re: [PATCH 3/3] xfs: optimise CRC updates
fs/xfs/libxfs/xfs_cksum.h xfs_start_cksum(char *buffer, size_t length, unsigned long cksum_offset) · line 34
Quoted 3 lines
fs/xfs/libxfs/xfs_cksum.h +2
-0+xfs_start_cksum_safe(char *buffer, size_t length, unsigned long cksum_offset) +xfs_start_cksum_update(char *buffer, size_t length, unsigned long cksum_offset)
What about turning the first argument into a void * for both so that we can avoid all the pointless casts?
Otherwise looks good:
Re: [PATCH 3/3] xfs: optimise CRC updates
Quoted 6 lines · Christoph Hellwig
fs/xfs/libxfs/xfs_cksum.h xfs_start_cksum(char *buffer, size_t length, unsigned long cksum_offset) · line 34
Quoted 3 lines
fs/xfs/libxfs/xfs_cksum.h +2
-0+xfs_start_cksum_safe(char *buffer, size_t length, unsigned long cksum_offset) +xfs_start_cksum_update(char *buffer, size_t length, unsigned long cksum_offset)What about turning the first argument into a void * for both so that we can avoid all the pointless casts?
I thought the C standard specifically disallowed pointer arithmetic on void pointers because they are an incomplete type, even though they have the same alignment/representation as a char *.
IIRC, gcc will throw errors on such code if -pedantic-errors or -Werror-pointer-arith is specified...
Cheers,
Dave.
Signature · 2 lines
Dave Chinner [email protected]
Re: [PATCH 3/3] xfs: optimise CRC updates
Quoted 6 lines · Dave Chinner
I thought the C standard specifically disallowed pointer arithmetic on void pointers because they are an incomplete type, even though they have the same alignment/representation as a char *.
IIRC, gcc will throw errors on such code if -pedantic-errors or -Werror-pointer-arith is specified...
void pointer arithmetics is a GNU extension indeed, but on that the kernel relies on heavily.