Miscellaneous bug fixes
cover Miscellaneous bug fixes
The following patches fix recently reported bugs and regressions. All except the log ticket patch should be considered for immediate inclusion.
PATCH 1/5 XFS: fix error inversion problems with data flushing
XFS gets the sign of the error wrong in several places when gathering the error from generic linux functions. These functions return negative error values, while the core XFS code returns positive error values. Hence when XFS inverts the error to be returned to the VFS, it can incorrectly invert a negative error and this error will be ignored by the syscall return.
Fix all the problems related to calling filemap_* functions.
Problem initially identified by Nick Piggin in xfs_fsync().
--- fs/xfs/linux-2.6/xfs_fs_subr.c | 23 ++++++++++++++++++++--- fs/xfs/linux-2.6/xfs_lrw.c | 2 +- fs/xfs/linux-2.6/xfs_super.c | 13 +++++++++---- fs/xfs/xfs_vnodeops.c | 2 +- fs/xfs/xfs_vnodeops.h | 1 + 5 files changed, 32 insertions(+), 9 deletions(-)
fs/xfs/linux-2.6/xfs_fs_subr.c +20 -3
@@ -24,6 +24,10 @@ int fs_noerr(void) { return 0; }int fs_nosys (void ) {return ENOSYS ; }void fs_noval (void ) {return ; } +/* +* note: all filemap functions return negative error codes. These +* need to be inverted before returning to the xfs core functions. +*/ void xfs_tosspages( xfs_inode_t *ip,
@@ -53,7 +57,7 @@ xfs_flushinval_pages(if (!ret)truncate_inode_pages (mapping, first); } -return ret; +return -ret; }int
@@ -72,10 +76,23 @@ xfs_flush_pages(xfs_iflags_clear (ip,XFS_ITRUNCATED ); ret =filemap_fdatawrite (mapping);if (flags &XFS_B_ASYNC ) -return ret; +return -ret; ret2 =filemap_fdatawait (mapping);if (!ret) ret = ret2; } -return ret; +return -ret; +} + +int +xfs_wait_on_pages ( +xfs_inode_t *ip, +xfs_off_t first, +xfs_off_t last) +{ +struct address_space *mapping =VFS_I (ip)->i_mapping; + +if (mapping_tagged (mapping,PAGECACHE_TAG_WRITEBACK )) +return -filemap_fdatawait (mapping); +return 0 ; }
fs/xfs/linux-2.6/xfs_lrw.c +1 -1
@@ -243,7 +243,7 @@ xfs_read(if (unlikely (ioflags &IO_ISDIRECT )) {if (inode->i_mapping->nrpages) - ret =xfs_flushinval_pages (ip, (*offset &PAGE_CACHE_MASK ), + ret = -xfs_flushinval_pages (ip, (*offset &PAGE_CACHE_MASK ),-1 ,FI_REMAPF_LOCKED );mutex_unlock (&inode->i_mutex);if (ret) {
fs/xfs/linux-2.6/xfs_super.c +9 -4
@@ -1012,21 +1012,26 @@ xfs_fs_write_inode( struct inode *inode, int sync) { +struct xfs_inode *ip =XFS_I (inode);int error =0 ;int flags =0 ; -xfs_itrace_entry (XFS_I (inode)); +xfs_itrace_entry (ip);if (sync) { -filemap_fdatawait (inode->i_mapping); + error =xfs_wait_on_pages (ip,0 ,-1 ); +if (error) + goto out_error; flags |=FLUSH_SYNC ; } - error =xfs_inode_flush (XFS_I (inode), flags); + error =xfs_inode_flush (ip, flags); + +out_error:/* * if we failed to write out the inode then mark * it dirty again so we'll try again later. */ if (error) -xfs_mark_inode_dirty_sync (XFS_I (inode)); +xfs_mark_inode_dirty_sync (ip);return -error; }
fs/xfs/xfs_vnodeops.c +1 -1
@@ -713,7 +713,7 @@ xfs_fsync(return XFS_ERROR (EIO );/* capture size updates in I/O completion before writing the inode. */ - error =filemap_fdatawait (VFS_I (ip)->i_mapping); + error =xfs_wait_on_pages (ip,0 ,-1 );if (error)return XFS_ERROR (error);
fs/xfs/xfs_vnodeops.h +1 -0
@@ -78,5 +78,6 @@ int xfs_flushinval_pages(struct xfs_inode *ip, xfs_off_t first, xfs_off_t last, int fiopt);int xfs_flush_pages (struct xfs_inode *ip,xfs_off_t first,xfs_off_t last,uint64_t flags,int fiopt); +int xfs_wait_on_pages (struct xfs_inode *ip,xfs_off_t first,xfs_off_t last);#endif /* _XFS_VNODEOPS_H */
Signature · 1 line
1.5.6.5
Re: [PATCH 1/5] XFS: fix error inversion problems with data flushing
Quoted 10 lines · Dave Chinner
XFS gets the sign of the error wrong in several places when gathering the error from generic linux functions. These functions return negative error values, while the core XFS code returns positive error values. Hence when XFS inverts the error to be returned to the VFS, it can incorrectly invert a negative error and this error will be ignored by the syscall return.
Fix all the problems related to calling filemap_* functions.
Problem initially identified by Nick Piggin in
xfs_fsync().
But this patch is a little more than just the error code inversions..
Re: [PATCH 1/5] XFS: fix error inversion problems with data flushing
Quoted 8 lines · Christoph Hellwig
Quoted 7 lines · Dave Chinner
XFS gets the sign of the error wrong in several places when gathering the error from generic linux functions. These functions return negative error values, while the core XFS code returns positive error values. Hence when XFS inverts the error to be returned to the VFS, it can incorrectly invert a negative error and this error will be ignored by the syscall return.
Quoted 5 lines
Quoted 3 lines
Fix all the problems related to calling filemap_* functions.
Problem initially identified by Nick Piggin in
xfs_fsync().But this patch is a little more than just the error code inversions..
Yes, that's the point of the patch - we get it wrong in many places and this fixes them so that correctly signed error values are returned to the higher layers....
Cheers,
Dave.
Signature · 2 lines
Dave Chinner [email protected]
Re: [PATCH 1/5] XFS: fix error inversion problems with data flushing
Quoted 3 lines · Dave Chinner
Yes, that's the point of the patch - we get it wrong in many places and this fixes them so that correctly signed error values are returned to the higher layers....
Well, you also introduce xfs_wait_on_pages, which should at least be documented. And I'd have to say I don't really like adding more of these wrappers just for the sake of it.
PATCH 2/5 XFS: Fix double free of log tickets
In reply to Dave ChinnerWhen an I/O error occurs during an intermediate commit on a rolling transaction, xfs_trans_commit() will free the transaction structure and the related ticket. However, the duplicate transaction that gets used as the transaction continues still contains a pointer to the ticket. Hence when the duplicate transaction is cancelled and freed, we free the ticket a second time.
Add reference counting to the ticket so that we hold an extra reference to the ticket over the transaction commit. We drop the extra reference once we have checked that the transaction commit did not return an error, thus avoiding a double free on commit error.
Credit to Nick Piggin for tripping over the problem.
--- fs/xfs/xfs_bmap.c | 10 ++++++++-- fs/xfs/xfs_inode.c | 10 ++++++++-- fs/xfs/xfs_log.c | 39 +++++++++++++++++++++++++-------------- fs/xfs/xfs_log.h | 4 ++++ fs/xfs/xfs_log_priv.h | 1 + fs/xfs/xfs_trans.c | 9 ++++++++- fs/xfs/xfs_utils.c | 6 ++++++ fs/xfs/xfs_vnodeops.c | 6 ++++++ 8 files changed, 66 insertions(+), 19 deletions(-)
fs/xfs/xfs_bmap.c +8 -2
@@ -4292,9 +4292,15 @@ xfs_bmap_finish(* We have a new transaction, so we should return committed=1, * even though we're returning an error. */ -if (error) { +if (error)return error; - } + +/* +* transaction commit worked ok so we can drop the extra ticket +* reference that we gained in xfs_trans_dup() +*/ +xfs_log_ticket_put (ntp->t_ticket); +if ((error =xfs_trans_reserve (ntp,0 , logres,0 ,XFS_TRANS_PERM_LOG_RES , logcount)))return error;
fs/xfs/xfs_inode.c +8 -2
@@ -1782,8 +1782,14 @@ xfs_itruncate_finish(xfs_trans_ijoin (ntp, ip,XFS_ILOCK_EXCL |XFS_IOLOCK_EXCL );xfs_trans_ihold (ntp, ip); -if (!error) - error =xfs_trans_reserve (ntp,0 , +if (error) +return error; +/* +* transaction commit worked ok so we can drop the extra ticket +* reference that we gained in xfs_trans_dup() +*/ +xfs_log_ticket_put (ntp->t_ticket); + error =xfs_trans_reserve (ntp,0 ,XFS_ITRUNCATE_LOG_RES (mp),0 ,XFS_TRANS_PERM_LOG_RES ,XFS_ITRUNCATE_LOG_COUNT );
fs/xfs/xfs_log.c +25 -14
@@ -100,12 +100,11 @@ STATIC void xlog_ungrant_log_space(xlog_t *log,/* local ticket functions */ -STATIC xlog_ticket_t *xlog_ticket_get(xlog_t *log, +STATIC xlog_ticket_t *xlog_ticket_alloc(xlog_t *log,int unit_bytes,int count,char clientid,uint flags); -STATIC void xlog_ticket_put(xlog_t *log, xlog_ticket_t *ticket);#if defined(DEBUG ) STATIC void xlog_verify_dest_ptr(xlog_t *log, __psint_t ptr);
@@ -360,7 +359,7 @@ xfs_log_done(xfs_mount_t *mp,*/ xlog_trace_loggrant (log, ticket,"xfs_log_done: (non-permanent)" );xlog_ungrant_log_space (log, ticket); -xlog_ticket_put (log, ticket); +xfs_log_ticket_put (ticket); }else {xlog_trace_loggrant (log, ticket,"xfs_log_done: (permanent)" );xlog_regrant_reserve_log_space (log, ticket);
@@ -514,7 +513,7 @@ xfs_log_reserve(xfs_mount_t *mp, retval =xlog_regrant_write_log_space (log, internal_ticket); }else {/* may sleep if need to allocate more tickets */ - internal_ticket =xlog_ticket_get (log, unit_bytes, cnt, + internal_ticket =xlog_ticket_alloc (log, unit_bytes, cnt, client, flags);if (!internal_ticket)return XFS_ERROR (ENOMEM );
@@ -749,7 +748,7 @@ xfs_log_unmount_write(xfs_mount_t *mp)if (tic) {xlog_trace_loggrant (log, tic,"unmount rec" );xlog_ungrant_log_space (log, tic); -xlog_ticket_put (log, tic); +xfs_log_ticket_put (tic); } }else {/*
@@ -3223,22 +3222,33 @@ xlog_state_want_sync(xlog_t *log, xlog_in_core_t *iclog)*/ /* -* Free a used ticket. +* Free a used ticket when it's refcount falls to zero. */ -STATIC void -xlog_ticket_put (xlog_t *log, -xlog_ticket_t *ticket) +void +xfs_log_ticket_put ( +xlog_ticket_t *ticket) { -sv_destroy (&ticket->t_wait); -kmem_zone_free (xfs_log_ticket_zone, ticket); -}/* xlog_ticket_put */ +ASSERT (atomic_read (&ticket->t_ref) >0 ); +if (atomic_dec_and_test (&ticket->t_ref)) { +sv_destroy (&ticket->t_wait); +kmem_zone_free (xfs_log_ticket_zone, ticket); + } +} +xlog_ticket_t * +xfs_log_ticket_get ( +xlog_ticket_t *ticket) +{ +ASSERT (atomic_read (&ticket->t_ref) >0 ); +atomic_inc (&ticket->t_ref); +return ticket; +}/* * Allocate and initialise a new log ticket. */ STATIC xlog_ticket_t * -xlog_ticket_get(xlog_t *log, +xlog_ticket_alloc(xlog_t *log, int unit_bytes, int cnt, char client,
@@ -3309,6 +3319,7 @@ xlog_ticket_get(xlog_t *log, unit_bytes +=2 *BBSIZE ; } +atomic_set (&tic->t_ref,1 ); tic->t_unit_res = unit_bytes; tic->t_curr_res = unit_bytes; tic->t_cnt = cnt;
@@ -3324,7 +3335,7 @@ xlog_ticket_get(xlog_t *log,xlog_tic_reset_res (tic);return tic; -}/* xlog_ticket_get */ +}/******************************************************************************
fs/xfs/xfs_log.h +4 -0
@@ -134,6 +134,7 @@ typedef struct xfs_log_callback {#ifdef __KERNEL__/* Log manager interfaces */ struct xfs_mount ; +struct xlog_ticket ; xfs_lsn_t xfs_log_done(struct xfs_mount *mp, xfs_log_ticket_t ticket, void **iclog,
@@ -177,6 +178,9 @@ int xfs_log_need_covered(struct xfs_mount *mp);void xlog_iodone (struct xfs_buf *); +struct xlog_ticket *xfs_log_ticket_get (struct xlog_ticket *ticket); +void xfs_log_ticket_put (struct xlog_ticket *ticket); +#endif
fs/xfs/xfs_log_priv.h +1 -0
@@ -245,6 +245,7 @@ typedef struct xlog_ticket {struct xlog_ticket *t_next;/* :4|8 */ struct xlog_ticket *t_prev;/* :4|8 */ xlog_tid_t t_tid;/* transaction identifier : 4 */ +atomic_t t_ref;/* ticket reference count : 4 */ int t_curr_res;/* current reservation in bytes : 4 */ int t_unit_res;/* unit reservation in bytes : 4 */ char t_ocnt;/* original count : 1 */
fs/xfs/xfs_trans.c +8 -1
@@ -290,7 +290,7 @@ xfs_trans_dup(ASSERT (tp->t_ticket !=NULL ); ntp->t_flags =XFS_TRANS_PERM_LOG_RES | (tp->t_flags &XFS_TRANS_RESERVE ); - ntp->t_ticket = tp->t_ticket; + ntp->t_ticket =xfs_log_ticket_get (tp->t_ticket); ntp->t_blk_res = tp->t_blk_res - tp->t_blk_res_used; tp->t_blk_res = tp->t_blk_res_used; ntp->t_rtx_res = tp->t_rtx_res - tp->t_rtx_res_used;
@@ -1260,6 +1260,13 @@ xfs_trans_roll( trans = *tpp;/* +* transaction commit worked ok so we can drop the extra ticket +* reference that we gained in xfs_trans_dup() +*/ +xfs_log_ticket_put (trans->t_ticket); + + +/* * Reserve space in the log for th next transaction. * This also pushes items in the "AIL", the list of logged items, * out to disk if they are taking up space at the tail of the log
fs/xfs/xfs_utils.c +6 -0
@@ -172,6 +172,12 @@ xfs_dir_ialloc( *ipp =NULL ;return code; } + +/* +* transaction commit worked ok so we can drop the extra ticket +* reference that we gained in xfs_trans_dup() +*/ +xfs_log_ticket_put (tp->t_ticket); code =xfs_trans_reserve (tp,0 , log_res,0 ,XFS_TRANS_PERM_LOG_RES , log_count);/*
fs/xfs/xfs_vnodeops.c +6 -0
@@ -1029,6 +1029,12 @@ xfs_inactive_symlink_rmt( goto error0; }/* +* transaction commit worked ok so we can drop the extra ticket +* reference that we gained in xfs_trans_dup() +*/ +xfs_log_ticket_put (tp->t_ticket); + +/* * Remove the memory for extent descriptions (just bookkeeping). */ if (ip->i_df.if_bytes)
Signature · 1 line
1.5.6.5
Re: [PATCH 2/5] XFS: Fix double free of log tickets
Looks good to me. And given that it fixes a regression due to the conversion of the transaction cache to slab it should get some priority, maybe even for another 2.6.28 pull.
Quoted 273 lines · Dave Chinner
When an I/O error occurs during an intermediate commit on a rolling transaction,
xfs_trans_commit()will free the transaction structure and the related ticket. However, the duplicate transaction that gets used as the transaction continues still contains a pointer to the ticket. Hence when the duplicate transaction is cancelled and freed, we free the ticket a second time.Add reference counting to the ticket so that we hold an extra reference to the ticket over the transaction commit. We drop the extra reference once we have checked that the transaction commit did not return an error, thus avoiding a double free on commit error.
Credit to Nick Piggin for tripping over the problem.
Signed-off-by Dave Chinner <[email protected]>--- fs/xfs/xfs_bmap.c | 10 ++++++++-- fs/xfs/xfs_inode.c | 10 ++++++++-- fs/xfs/xfs_log.c | 39 +++++++++++++++++++++++++-------------- fs/xfs/xfs_log.h | 4 ++++ fs/xfs/xfs_log_priv.h | 1 + fs/xfs/xfs_trans.c | 9 ++++++++- fs/xfs/xfs_utils.c | 6 ++++++ fs/xfs/xfs_vnodeops.c | 6 ++++++ 8 files changed, 66 insertions(+), 19 deletions(-)fs/xfs/xfs_bmap.c +8
-2@@ -4292,9 +4292,15 @@ xfs_bmap_finish(* We have a new transaction, so we should return committed=1, * even though we're returning an error. */ -if (error) { +if (error)return error; - } + +/* +* transaction commit worked ok so we can drop the extra ticket +* reference that we gained in xfs_trans_dup() +*/ +xfs_log_ticket_put (ntp->t_ticket); +if ((error =xfs_trans_reserve (ntp,0 , logres,0 ,XFS_TRANS_PERM_LOG_RES , logcount)))return error;fs/xfs/xfs_inode.c +8
-2@@ -1782,8 +1782,14 @@ xfs_itruncate_finish(xfs_trans_ijoin (ntp, ip,XFS_ILOCK_EXCL |XFS_IOLOCK_EXCL );xfs_trans_ihold (ntp, ip); -if (!error) - error =xfs_trans_reserve (ntp,0 , +if (error) +return error; +/* +* transaction commit worked ok so we can drop the extra ticket +* reference that we gained in xfs_trans_dup() +*/ +xfs_log_ticket_put (ntp->t_ticket); + error =xfs_trans_reserve (ntp,0 ,XFS_ITRUNCATE_LOG_RES (mp),0 ,XFS_TRANS_PERM_LOG_RES ,XFS_ITRUNCATE_LOG_COUNT );fs/xfs/xfs_log.c +25
-14@@ -100,12 +100,11 @@ STATIC void xlog_ungrant_log_space(xlog_t *log,/* local ticket functions */ -STATIC xlog_ticket_t *xlog_ticket_get(xlog_t *log, +STATIC xlog_ticket_t *xlog_ticket_alloc(xlog_t *log,int unit_bytes,int count,char clientid,uint flags); -STATIC void xlog_ticket_put(xlog_t *log, xlog_ticket_t *ticket);#if defined(DEBUG ) STATIC void xlog_verify_dest_ptr(xlog_t *log, __psint_t ptr);@@ -360,7 +359,7 @@ xfs_log_done(xfs_mount_t *mp,*/ xlog_trace_loggrant (log, ticket,"xfs_log_done: (non-permanent)" );xlog_ungrant_log_space (log, ticket); -xlog_ticket_put (log, ticket); +xfs_log_ticket_put (ticket); }else {xlog_trace_loggrant (log, ticket,"xfs_log_done: (permanent)" );xlog_regrant_reserve_log_space (log, ticket);@@ -514,7 +513,7 @@ xfs_log_reserve(xfs_mount_t *mp, retval =xlog_regrant_write_log_space (log, internal_ticket); }else {/* may sleep if need to allocate more tickets */ - internal_ticket =xlog_ticket_get (log, unit_bytes, cnt, + internal_ticket =xlog_ticket_alloc (log, unit_bytes, cnt, client, flags);if (!internal_ticket)return XFS_ERROR (ENOMEM );@@ -749,7 +748,7 @@ xfs_log_unmount_write(xfs_mount_t *mp)if (tic) {xlog_trace_loggrant (log, tic,"unmount rec" );xlog_ungrant_log_space (log, tic); -xlog_ticket_put (log, tic); +xfs_log_ticket_put (tic); } }else {/* @@ -3223,22 +3222,33 @@ xlog_state_want_sync(xlog_t *log, xlog_in_core_t *iclog)*/ /* -* Free a used ticket. +* Free a used ticket when it's refcount falls to zero. */ -STATIC void -xlog_ticket_put (xlog_t *log, -xlog_ticket_t *ticket) +void +xfs_log_ticket_put ( +xlog_ticket_t *ticket) { -sv_destroy (&ticket->t_wait); -kmem_zone_free (xfs_log_ticket_zone, ticket); -}/* xlog_ticket_put */ +ASSERT (atomic_read (&ticket->t_ref) >0 ); +if (atomic_dec_and_test (&ticket->t_ref)) { +sv_destroy (&ticket->t_wait); +kmem_zone_free (xfs_log_ticket_zone, ticket); + } +} +xlog_ticket_t * +xfs_log_ticket_get ( +xlog_ticket_t *ticket) +{ +ASSERT (atomic_read (&ticket->t_ref) >0 ); +atomic_inc (&ticket->t_ref); +return ticket; +}/* * Allocate and initialise a new log ticket. */ STATIC xlog_ticket_t * -xlog_ticket_get(xlog_t *log, +xlog_ticket_alloc(xlog_t *log, int unit_bytes, int cnt, char client,@@ -3309,6 +3319,7 @@ xlog_ticket_get(xlog_t *log, unit_bytes +=2 *BBSIZE ; } +atomic_set (&tic->t_ref,1 ); tic->t_unit_res = unit_bytes; tic->t_curr_res = unit_bytes; tic->t_cnt = cnt;@@ -3324,7 +3335,7 @@ xlog_ticket_get(xlog_t *log,xlog_tic_reset_res (tic);return tic; -}/* xlog_ticket_get */ +}/****************************************************************************** fs/xfs/xfs_log.h +4
-0@@ -134,6 +134,7 @@ typedef struct xfs_log_callback {#ifdef __KERNEL__/* Log manager interfaces */ struct xfs_mount ; +struct xlog_ticket ; xfs_lsn_t xfs_log_done(struct xfs_mount *mp, xfs_log_ticket_t ticket, void **iclog,@@ -177,6 +178,9 @@ int xfs_log_need_covered(struct xfs_mount *mp);void xlog_iodone (struct xfs_buf *); +struct xlog_ticket *xfs_log_ticket_get (struct xlog_ticket *ticket); +void xfs_log_ticket_put (struct xlog_ticket *ticket); +#endif fs/xfs/xfs_log_priv.h +1
-0@@ -245,6 +245,7 @@ typedef struct xlog_ticket {struct xlog_ticket *t_next;/* :4|8 */ struct xlog_ticket *t_prev;/* :4|8 */ xlog_tid_t t_tid;/* transaction identifier : 4 */ +atomic_t t_ref;/* ticket reference count : 4 */ int t_curr_res;/* current reservation in bytes : 4 */ int t_unit_res;/* unit reservation in bytes : 4 */ char t_ocnt;/* original count : 1 */ fs/xfs/xfs_trans.c +8
-1@@ -290,7 +290,7 @@ xfs_trans_dup(ASSERT (tp->t_ticket !=NULL ); ntp->t_flags =XFS_TRANS_PERM_LOG_RES | (tp->t_flags &XFS_TRANS_RESERVE ); - ntp->t_ticket = tp->t_ticket; + ntp->t_ticket =xfs_log_ticket_get (tp->t_ticket); ntp->t_blk_res = tp->t_blk_res - tp->t_blk_res_used; tp->t_blk_res = tp->t_blk_res_used; ntp->t_rtx_res = tp->t_rtx_res - tp->t_rtx_res_used;@@ -1260,6 +1260,13 @@ xfs_trans_roll( trans = *tpp;/* +* transaction commit worked ok so we can drop the extra ticket +* reference that we gained in xfs_trans_dup() +*/ +xfs_log_ticket_put (trans->t_ticket); + + +/* * Reserve space in the log for th next transaction. * This also pushes items in the "AIL", the list of logged items, * out to disk if they are taking up space at the tail of the log fs/xfs/xfs_utils.c +6
-0@@ -172,6 +172,12 @@ xfs_dir_ialloc( *ipp =NULL ;return code; } + +/* +* transaction commit worked ok so we can drop the extra ticket +* reference that we gained in xfs_trans_dup() +*/ +xfs_log_ticket_put (tp->t_ticket); code =xfs_trans_reserve (tp,0 , log_res,0 ,XFS_TRANS_PERM_LOG_RES , log_count);/* fs/xfs/xfs_vnodeops.c +6
-0@@ -1029,6 +1029,12 @@ xfs_inactive_symlink_rmt( goto error0; }/* +* transaction commit worked ok so we can drop the extra ticket +* reference that we gained in xfs_trans_dup() +*/ +xfs_log_ticket_put (tp->t_ticket); + +/* * Remove the memory for extent descriptions (just bookkeeping). */ if (ip->i_df.if_bytes)Signature · 1 line
1.5.6.5
---end quoted text---
PATCH 3/5 XFS: fix uninitialised variable bug in dquot release.
In reply to Dave Chinnergcc on ARM warns about an using an uninitialised variable in xfs_qm_dqrele_all_inodes(). This is a real bug, but gcc on x86_64 is not reporting this warning so it went unnoticed.
Fix the bug by bring the inode radix tree walk code up to date with xfs_sync_inodes_ag().
--- fs/xfs/quota/xfs_qm_syscalls.c | 42 ++++++++++++++++++++++----------------- 1 files changed, 24 insertions(+), 18 deletions(-)
fs/xfs/quota/xfs_qm_syscalls.c +24 -18
@@ -1036,9 +1036,6 @@ xfs_qm_dqrele_inodes_ag(int nr_found; do { - boolean_t inode_refed; - struct inode *inode; - /* * use a gang lookup to find the next inode in the tree * as the tree is sparse and a gang lookup walks to find
@@ -1053,27 +1050,37 @@ xfs_qm_dqrele_inodes_ag(break ; } -/* update the index for the next lookup */ +/* +* Update the index for the next lookup. Catch overflows +* into the next AG range which can occur if we have inodes +* in the last block of the AG and we are currently +* pointing to the last inode. +*/ first_index =XFS_INO_TO_AGINO (mp, ip->i_ino +1 ); +if (first_index <XFS_INO_TO_AGINO (mp, ip->i_ino)) { +read_unlock (&pag->pag_ici_lock); +break ; + } -/* skip quota inodes and those in reclaim */ - inode =VFS_I (ip); -if (!inode || ip ==XFS_QI_UQIP (mp) || ip ==XFS_QI_GQIP (mp)) { +/* skip quota inodes */ +if (ip ==XFS_QI_UQIP (mp) || ip ==XFS_QI_GQIP (mp)) {ASSERT (ip->i_udquot ==NULL );ASSERT (ip->i_gdquot ==NULL );read_unlock (&pag->pag_ici_lock);continue ; } -if (xfs_ilock_nowait (ip,XFS_ILOCK_EXCL ) ==0 ) { - inode =igrab (inode); -read_unlock (&pag->pag_ici_lock); -if (!inode) -continue ; - inode_refed =B_TRUE ; -xfs_ilock (ip,XFS_ILOCK_EXCL ); - }else { + +/* +* If we can't get a reference on the inode, it must be +* in reclaim. Leave it for the reclaim code to flush. +*/ +if (!igrab (VFS_I (ip))) {read_unlock (&pag->pag_ici_lock); +continue ; } +read_unlock (&pag->pag_ici_lock); + +xfs_ilock (ip,XFS_ILOCK_EXCL );if ((flags &XFS_UQUOTA_ACCT ) && ip->i_udquot) {xfs_qm_dqrele (ip->i_udquot); ip->i_udquot =NULL ;
@@ -1083,9 +1090,8 @@ xfs_qm_dqrele_inodes_ag(xfs_qm_dqrele (ip->i_gdquot); ip->i_gdquot =NULL ; } -xfs_iunlock (ip,XFS_ILOCK_EXCL ); -if (inode_refed) -IRELE (ip); +xfs_iput (ip,XFS_ILOCK_EXCL ); + }while (nr_found); }
Signature · 1 line
1.5.6.5
Re: [PATCH 3/5] XFS: fix uninitialised variable bug in dquot release.
Quoted 6 lines · Dave Chinner
gcc on ARM warns about an using an uninitialised variable in
xfs_qm_dqrele_all_inodes(). This is a real bug, but gcc onx86_64is not reporting this warning so it went unnoticed.Fix the bug by bring the inode radix tree walk code up to date with
xfs_sync_inodes_ag().
Looks good.
PATCH 4/5 XFS: fix spurious uninitialised variable warning in xfs_growfs_rt
In reply to Dave Chinnergcc is warning about an uninitialised variable in xfs_growfs_rt(). This is a false positive. Fix it by changing the scope of the transaction pointer to wholly within the internal loop inside the function.
While there, preemptively change xfs_growfs_rt_alloc() in the same way as it has exactly the same structure as xfs_growfs_rt() but gcc is not warning about it. Yet.
--- fs/xfs/xfs_rtalloc.c | 39 ++++++++++++++++++--------------------- 1 files changed, 18 insertions(+), 21 deletions(-)
fs/xfs/xfs_rtalloc.c +18 -21
@@ -85,7 +85,6 @@ xfs_growfs_rt_alloc( {xfs_fileoff_t bno;/* block number in file */ xfs_buf_t *bp;/* temporary buffer for zeroing */ -int cancelflags;/* flags for xfs_trans_cancel */ int committed;/* transaction committed flag */ xfs_daddr_t d;/* disk block address */ int error;/* error return value */
@@ -96,15 +95,16 @@ xfs_growfs_rt_alloc(xfs_bmbt_irec_t map;/* block map output */ int nmap;/* number of block maps */ int resblks;/* space reservation */ -xfs_trans_t *tp;/* transaction pointer */ /* * Allocate space to the file, as necessary. */ while (oblocks < nblocks) { +int cancelflags =0 ; +xfs_trans_t *tp; + tp =xfs_trans_alloc (mp,XFS_TRANS_GROWFSRT_ALLOC ); resblks =XFS_GROWFSRT_SPACE_RES (mp, nblocks - oblocks); - cancelflags =0 ;/* * Reserve space & log for one extent added to the file. */
@@ -171,7 +171,9 @@ xfs_growfs_rt_alloc( mp->m_bsize, 0);if (bp ==NULL ) { error =XFS_ERROR (EIO ); - goto error_cancel; +error_cancel: +xfs_trans_cancel (tp, cancelflags); + goto error; }memset (XFS_BUF_PTR (bp),0 , mp->m_sb.sb_blocksize);xfs_trans_log_buf (tp, bp,0 , mp->m_sb.sb_blocksize -1 );
@@ -188,8 +190,6 @@ xfs_growfs_rt_alloc( oblocks = map.br_startoff + map.br_blockcount; }return 0 ; -error_cancel: -xfs_trans_cancel (tp, cancelflags); error:return error; }
@@ -1856,7 +1856,6 @@ xfs_growfs_rt( {xfs_rtblock_t bmbno;/* bitmap block number */ xfs_buf_t *bp;/* temporary buffer */ -int cancelflags;/* flags for xfs_trans_cancel */ int error;/* error return value */ xfs_inode_t *ip;/* bitmap inode, used as lock */ xfs_mount_t *nmp;/* new (fake) mount structure */
@@ -1872,10 +1871,8 @@ xfs_growfs_rt(xfs_extlen_t rsumblocks;/* current number of rt summary blks */ xfs_sb_t *sbp;/* old superblock */ xfs_fsblock_t sumbno;/* summary block number */ -xfs_trans_t *tp;/* transaction pointer */ sbp = &mp->m_sb; - cancelflags =0 ;/* * Initial error checking. */
@@ -1942,6 +1939,9 @@ xfs_growfs_rt( ((sbp->sb_rextents & ((1 << mp->m_blkbit_log) -1 )) !=0 ); bmbno < nrbmblocks; bmbno++) { +xfs_trans_t *tp; +int cancelflags =0 ; + *nmp = *mp; nsbp = &nmp->m_sb;/*
@@ -1967,16 +1967,15 @@ xfs_growfs_rt(* Start a transaction, get the log reservation. */ tp =xfs_trans_alloc (mp,XFS_TRANS_GROWFSRT_FREE ); - cancelflags =0 ;if ((error =xfs_trans_reserve (tp,0 ,XFS_GROWRTFREE_LOG_RES (nmp),0 ,0 ,0 ))) -break ; + goto error_cancel;/* * Lock out other callers by grabbing the bitmap inode lock. */ if ((error =xfs_trans_iget (mp, tp, mp->m_sb.sb_rbmino,0 ,XFS_ILOCK_EXCL , &ip))) -break ; + goto error_cancel;ASSERT (ip == mp->m_rbmip);/* * Update the bitmap inode's size.
@@ -1990,7 +1989,7 @@ xfs_growfs_rt(*/ if ((error =xfs_trans_iget (mp, tp, mp->m_sb.sb_rsumino,0 ,XFS_ILOCK_EXCL , &ip))) -break ; + goto error_cancel;ASSERT (ip == mp->m_rsumip);/* * Update the summary inode's size.
@@ -2005,7 +2004,7 @@ xfs_growfs_rt( mp->m_rsumlevels != nmp->m_rsumlevels) { error =xfs_rtcopy_summary (mp, nmp, tp);if (error) -break ; + goto error_cancel; }/* * Update superblock fields.
@@ -2031,8 +2030,11 @@ xfs_growfs_rt( bp =NULL ; error =xfs_rtfree_range (nmp, tp, sbp->sb_rextents, nsbp->sb_rextents - sbp->sb_rextents, &bp, &sumbno); -if (error) +if (error) { +error_cancel: +xfs_trans_cancel (tp, cancelflags);break ; + }/* * Mark more blocks free in the superblock. */
@@ -2045,15 +2047,10 @@ xfs_growfs_rt( mp->m_rsumsize = nrsumsize; error =xfs_trans_commit (tp,0 ); -if (error) { - tp =NULL ; +if (error)break ; - } } -if (error && tp) -xfs_trans_cancel (tp, cancelflags); -/* * Free the fake mp structure. */
Signature · 1 line
1.5.6.5
Re: [PATCH 4/5] XFS: fix spurious uninitialised variable warning in xfs_growfs_rt
Quoted 8 lines · Dave Chinner
gcc is warning about an uninitialised variable in
xfs_growfs_rt(). This is a false positive. Fix it by changing the scope of the transaction pointer to wholly within the internal loop inside the function.While there, preemptively change
xfs_growfs_rt_alloc()in the same way as it has exactly the same structure asxfs_growfs_rt()but gcc is not warning about it. Yet.
Looks good.
PATCH 5/5 XFS: Avoid using inodes that haven't been completely initialised
In reply to Dave ChinnerThe radix tree walks in xfs_sync_inodes_ag and xfs_qm_dqrele_all_inodes() can find inodes that are still undergoing initialisation. Avoid them by checking for the the XFS_INEW() flag once we have a reference on the inode. This flag is cleared once the inode is properly initialised.
--- fs/xfs/linux-2.6/xfs_iops.c | 1 - fs/xfs/linux-2.6/xfs_sync.c | 5 +++-- fs/xfs/quota/xfs_qm_syscalls.c | 6 ++++++ 3 files changed, 9 insertions(+), 3 deletions(-)
fs/xfs/linux-2.6/xfs_iops.c +0 -1
@@ -772,7 +772,6 @@ xfs_setup_inode( inode->i_ino = ip->i_ino; inode->i_state =I_NEW |I_LOCK ;inode_add_to_lists (ip->i_mount->m_super, inode); -ASSERT (atomic_read (&inode->i_count) ==1 ); inode->i_mode = ip->i_d.di_mode; inode->i_nlink = ip->i_d.di_nlink;
fs/xfs/linux-2.6/xfs_sync.c +3 -2
@@ -117,8 +117,9 @@ xfs_sync_inodes_ag( }read_unlock (&pag->pag_ici_lock); -/* bad inodes are dealt with elsewhere */ -if (is_bad_inode (inode)) { +/* avoid new or bad inodes */ +if (is_bad_inode (inode) || +xfs_iflags_test (ip,XFS_INEW )) {IRELE (ip);continue ; }
fs/xfs/quota/xfs_qm_syscalls.c +6 -0
@@ -1080,6 +1080,12 @@ xfs_qm_dqrele_inodes_ag( }read_unlock (&pag->pag_ici_lock); +/* avoid new inodes though we shouldn't find any here */ +if (xfs_iflags_test (ip,XFS_INEW )) { +IRELE (ip); +continue ; + } +xfs_ilock (ip,XFS_ILOCK_EXCL );if ((flags &XFS_UQUOTA_ACCT ) && ip->i_udquot) {xfs_qm_dqrele (ip->i_udquot);
Signature · 1 line
1.5.6.5
Re: [PATCH 5/5] XFS: Avoid using inodes that haven't been completely initialised
Quoted 4 lines · Dave Chinner
The radix tree walks in
xfs_sync_inodes_agandxfs_qm_dqrele_all_inodes()can find inodes that are still undergoing initialisation. Avoid them by checking for the theXFS_INEW()flag once we have a reference on the inode. This flag is cleared once the inode is properly initialised.
Yeah, that's what I meant with my initial comment that we can grab the inodes anytime they are on the radix tree.
Looks good.