Log and transaction cleanups, factoring and bug fixes V2
cover Log and transaction cleanups, factoring and bug fixes V2
The following series is preparation for delayed logging patches. The cleanups and factoring patches are mainly to make it simple to introduce the delayed logging patches, especially as these changes really have nothing to do with delayed logging. The cleanups and factoring patches should be no-ops from a functionality point of view.
A couple of the patches are fixes for bugs that I've found as I've been working through delayed logging prototypes.
Patches 8 (reservation bug fix) and 9 (new cleanup) of the series still require review.
Version 2:
- drop log vector changes from the series
- fix bogus assert in xfs_trans_committed() changes
- Added Christoph's _xfs_trans_commit() cleanup patch
- Added further cleanup of the xfs_trans_committed() code as suggested by Christoph.
These changes are also available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/dgc/xfs.git trans-cleanup
Christoph Hellwig (1):
xfs: clean up xfs_trans_commit logic even more
Dave Chinner (8):
xfs: factor log item initialisation
xfs: Add inode pin counts to traces
xfs: remove stale parameter from ->iop_unpin method
xfs: fix reservation release commit flag in xfs_bmap_add_attrfork()
xfs: split out iclog writing from xfs_trans_commit()
xfs: update and factor xfs_trans_committed()
xfs: Clean up xfs_trans_committed code after factoring
xfs: log ticket reservation underestimates the number of iclogs
fs/xfs/linux-2.6/xfs_trace.h | 9 +- fs/xfs/quota/xfs_dquot_item.c | 28 +- fs/xfs/xfs_bmap.c | 2 +- fs/xfs/xfs_buf_item.c | 55 ++-- fs/xfs/xfs_extfree_item.c | 18 +- fs/xfs/xfs_inode.c | 2 + fs/xfs/xfs_inode_item.c | 21 +- fs/xfs/xfs_log.c | 68 +++- fs/xfs/xfs_log.h | 7 + fs/xfs/xfs_trans.c | 760 +++++++++++++++++++---------------------- fs/xfs/xfs_trans.h | 5 +- fs/xfs/xfs_trans_buf.c | 3 +- 12 files changed, 481 insertions(+), 497 deletions(-)
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 1/9 xfs: factor log item initialisation
From: Dave Chinner <[email protected]>
Each log item type does manual initialisation of the log item. Delayed logging introduceѕ new fields that need initialisation, so factor all the open coded initialisation into a common function first.
--- fs/xfs/quota/xfs_dquot_item.c | 12 ++++-------- fs/xfs/xfs_buf_item.c | 5 +---- fs/xfs/xfs_extfree_item.c | 10 ++-------- fs/xfs/xfs_inode_item.c | 12 ++---------- fs/xfs/xfs_log.c | 13 +++++++++++++ fs/xfs/xfs_log.h | 7 +++++++ 6 files changed, 29 insertions(+), 30 deletions(-)
fs/xfs/quota/xfs_dquot_item.c +4 -8
@@ -357,9 +357,8 @@ xfs_qm_dquot_logitem_init(xfs_dq_logitem_t *lp; lp = &dqp->q_logitem; - lp->qli_item.li_type =XFS_LI_DQUOT ; - lp->qli_item.li_ops = &xfs_dquot_item_ops; - lp->qli_item.li_mountp = dqp->q_mount; +xfs_log_item_init (dqp->q_mount, &lp->qli_item,XFS_LI_DQUOT , + &xfs_dquot_item_ops); lp->qli_dquot = dqp; lp->qli_format.qlf_type =XFS_LI_DQUOT ; lp->qli_format.qlf_id =be32_to_cpu (dqp->q_core.d_id);
@@ -586,11 +585,8 @@ xfs_qm_qoff_logitem_init( qf = (xfs_qoff_logitem_t *)kmem_zalloc (sizeof (xfs_qoff_logitem_t),KM_SLEEP ); - qf->qql_item.li_type =XFS_LI_QUOTAOFF ; -if (start) - qf->qql_item.li_ops = &xfs_qm_qoffend_logitem_ops; -else - qf->qql_item.li_ops = &xfs_qm_qoff_logitem_ops; +xfs_log_item_init (mp, &qf->qql_item,XFS_LI_QUOTAOFF , start ? + &xfs_qm_qoffend_logitem_ops : &xfs_qm_qoff_logitem_ops); qf->qql_item.li_mountp = mp; qf->qql_format.qf_type =XFS_LI_QUOTAOFF ; qf->qql_format.qf_flags = flags;
fs/xfs/xfs_buf_item.c +1 -4
@@ -733,10 +733,7 @@ xfs_buf_item_init( bip = (xfs_buf_log_item_t *)kmem_zone_zalloc (xfs_buf_item_zone,KM_SLEEP ); - bip->bli_item.li_type =XFS_LI_BUF ; - bip->bli_item.li_ops = &xfs_buf_item_ops; - bip->bli_item.li_mountp = mp; - bip->bli_item.li_ailp = mp->m_ail; +xfs_log_item_init (mp, &bip->bli_item,XFS_LI_BUF , &xfs_buf_item_ops); bip->bli_buf = bp;xfs_buf_hold (bp); bip->bli_format.blf_type =XFS_LI_BUF ;
fs/xfs/xfs_extfree_item.c +2 -8
@@ -259,10 +259,7 @@ xfs_efi_init(xfs_mount_t *mp, KM_SLEEP); } - efip->efi_item.li_type =XFS_LI_EFI ; - efip->efi_item.li_ops = &xfs_efi_item_ops; - efip->efi_item.li_mountp = mp; - efip->efi_item.li_ailp = mp->m_ail; +xfs_log_item_init (mp, &efip->efi_item,XFS_LI_EFI , &xfs_efi_item_ops); efip->efi_format.efi_nextents = nextents; efip->efi_format.efi_id = (__psint_t )(void *)efip;
@@ -554,10 +551,7 @@ xfs_efd_init(xfs_mount_t *mp, KM_SLEEP); } - efdp->efd_item.li_type =XFS_LI_EFD ; - efdp->efd_item.li_ops = &xfs_efd_item_ops; - efdp->efd_item.li_mountp = mp; - efdp->efd_item.li_ailp = mp->m_ail; +xfs_log_item_init (mp, &efdp->efd_item,XFS_LI_EFD , &xfs_efd_item_ops); efdp->efd_efip = efip; efdp->efd_format.efd_nextents = nextents; efdp->efd_format.efd_efi_id = efip->efi_format.efi_id;
fs/xfs/xfs_inode_item.c +2 -10
@@ -865,17 +865,9 @@ xfs_inode_item_init(ASSERT (ip->i_itemp ==NULL ); iip = ip->i_itemp =kmem_zone_zalloc (xfs_ili_zone,KM_SLEEP ); - iip->ili_item.li_type =XFS_LI_INODE ; - iip->ili_item.li_ops = &xfs_inode_item_ops; - iip->ili_item.li_mountp = mp; - iip->ili_item.li_ailp = mp->m_ail; iip->ili_inode = ip; - -/* -We have zeroed memory. No need ... -iip->ili_extents_buf = NULL; -*/ - +xfs_log_item_init (mp, &iip->ili_item,XFS_LI_INODE , + &xfs_inode_item_ops); iip->ili_format.ilf_type =XFS_LI_INODE ; iip->ili_format.ilf_ino = ip->i_ino; iip->ili_format.ilf_blkno = ip->i_imap.im_blkno;
fs/xfs/xfs_log.c +13 -0
@@ -648,6 +648,19 @@ xfs_log_unmount(xfs_mount_t *mp)xlog_dealloc_log (mp->m_log); } +void +xfs_log_item_init ( +struct xfs_mount *mp, +struct xfs_log_item *item, +int type, +struct xfs_item_ops *ops) +{ + item->li_mountp = mp; + item->li_ailp = mp->m_ail; + item->li_type = type; + item->li_ops = ops; +} +/* * Write region vectors to log. The write happens using the space reservation * of the ticket (tic). It is not a requirement that all writes for a given
fs/xfs/xfs_log.h +7 -0
@@ -126,6 +126,13 @@ typedef struct xfs_log_callback {struct xfs_mount ;struct xlog_in_core ;struct xlog_ticket ; +struct xfs_log_item ; +struct xfs_item_ops ; + +void xfs_log_item_init (struct xfs_mount *mp, +struct xfs_log_item *item, +int type, +struct xfs_item_ops *ops); xfs_lsn_t xfs_log_done(struct xfs_mount *mp, struct xlog_ticket *ticket,
Signature · 6 lines
1.6.5 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
PATCH 2/9 xfs: Add inode pin counts to traces
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
We don't record pin counts in inode events right now, and this makes it difficult to track down problems related to pinning inodes. Add the pin count to the inode trace class and add trace events for pinning and unpinning inodes.
--- fs/xfs/linux-2.6/xfs_trace.h | 9 ++++++++- fs/xfs/xfs_inode.c | 2 ++ fs/xfs/xfs_inode_item.c | 2 ++ 3 files changed, 12 insertions(+), 1 deletions(-)
fs/xfs/linux-2.6/xfs_trace.h +8 -1
@@ -562,18 +562,21 @@ DECLARE_EVENT_CLASS(xfs_inode_class, __field(dev_t, dev) __field(xfs_ino_t, ino) __field(int, count) + __field(int, pincount) __field(unsigned long, caller_ip) ),TP_fast_assign ( __entry->dev = VFS_I(ip)->i_sb->s_dev; __entry->ino = ip->i_ino; __entry->count =atomic_read (&VFS_I (ip)->i_count); + __entry->pincount =atomic_read (&ip->i_pincount); __entry->caller_ip = caller_ip; ), -TP_printk ("dev %d:%d ino 0x%llx count %d caller %pf" , +TP_printk ("dev %d:%d ino 0x%llx count %d pincount %d caller %pf" ,MAJOR (__entry->dev),MINOR (__entry->dev), __entry->ino, __entry->count, + __entry->pincount, (char *)__entry->caller_ip) )
@@ -583,6 +586,10 @@ DEFINE_EVENT(xfs_inode_class, name, \ TP_ARGS(ip, caller_ip))DEFINE_INODE_EVENT (xfs_ihold);DEFINE_INODE_EVENT (xfs_irele); +DEFINE_INODE_EVENT (xfs_inode_pin); +DEFINE_INODE_EVENT (xfs_inode_unpin); +DEFINE_INODE_EVENT (xfs_inode_unpin_nowait); +/* the old xfs_itrace_entry tracer - to be replaced by s.th. in the VFS */ DEFINE_INODE_EVENT (xfs_inode);#define xfs_itrace_entry (ip) \
fs/xfs/xfs_inode.c +2 -0
@@ -2449,6 +2449,8 @@ xfs_iunpin_nowait( {ASSERT (xfs_isilocked (ip,XFS_ILOCK_EXCL |XFS_ILOCK_SHARED )); +trace_xfs_inode_unpin_nowait (ip, _RET_IP_); +/* Give the log a push to start the unpinning I/O */ xfs_log_force_lsn (ip->i_mount, ip->i_itemp->ili_last_lsn,0 );
fs/xfs/xfs_inode_item.c +2 -0
@@ -543,6 +543,7 @@ xfs_inode_item_pin( {ASSERT (xfs_isilocked (iip->ili_inode,XFS_ILOCK_EXCL )); +trace_xfs_inode_pin (iip->ili_inode, _RET_IP_);atomic_inc (&iip->ili_inode->i_pincount); }
@@ -561,6 +562,7 @@ xfs_inode_item_unpin( {struct xfs_inode *ip = iip->ili_inode; +trace_xfs_inode_unpin (ip, _RET_IP_);ASSERT (atomic_read (&ip->i_pincount) >0 );if (atomic_dec_and_test (&ip->i_pincount))wake_up (&ip->i_ipin_wait);
Signature · 6 lines
1.6.5 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
PATCH 3/9 xfs: remove stale parameter from ->iop_unpin method
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
The staleness of a object being unpinned can be directly derived from the object itself - there is no need to extract it from the object then pass it as a parameter into IOP_UNPIN().
This means we can kill the XFS_LID_BUF_STALE flag - it is set, checked and cleared in the same places XFS_BLI_STALE flag in the xfs_buf_log_item so it is now redundant and hence safe to remove.
--- fs/xfs/quota/xfs_dquot_item.c | 16 +++++-------- fs/xfs/xfs_buf_item.c | 50 ++++++++++++++++++----------------------- fs/xfs/xfs_extfree_item.c | 8 +++--- fs/xfs/xfs_inode_item.c | 7 ++--- fs/xfs/xfs_trans.c | 2 +- fs/xfs/xfs_trans.h | 5 +-- fs/xfs/xfs_trans_buf.c | 3 +- 7 files changed, 39 insertions(+), 52 deletions(-)
fs/xfs/quota/xfs_dquot_item.c +6 -10
@@ -107,8 +107,7 @@ xfs_qm_dquot_logitem_pin(/* ARGSUSED */ STATIC voidxfs_qm_dquot_logitem_unpin ( -xfs_dq_logitem_t *logitem, -int stale) +xfs_dq_logitem_t *logitem) {xfs_dquot_t *dqp = logitem->qli_dquot;
@@ -123,7 +122,7 @@ xfs_qm_dquot_logitem_unpin_remove( xfs_dq_logitem_t *logitem, xfs_trans_t *tp) { -xfs_qm_dquot_logitem_unpin (logitem,0 ); +xfs_qm_dquot_logitem_unpin (logitem); }/*
@@ -329,8 +328,7 @@ static struct xfs_item_ops xfs_dquot_item_ops = { .iop_format = (void(*)(xfs_log_item_t*, xfs_log_iovec_t*)) xfs_qm_dquot_logitem_format, .iop_pin = (void(*)(xfs_log_item_t*))xfs_qm_dquot_logitem_pin, - .iop_unpin = (void (*)(xfs_log_item_t *,int )) - xfs_qm_dquot_logitem_unpin, + .iop_unpin = (void(*)(xfs_log_item_t*))xfs_qm_dquot_logitem_unpin, .iop_unpin_remove = (void (*)(xfs_log_item_t *,xfs_trans_t *)) xfs_qm_dquot_logitem_unpin_remove, .iop_trylock = (uint(*)(xfs_log_item_t*))
@@ -425,7 +423,7 @@ xfs_qm_qoff_logitem_pin(xfs_qoff_logitem_t *qf)*/ /*ARGSUSED*/ STATIC void -xfs_qm_qoff_logitem_unpin (xfs_qoff_logitem_t *qf,int stale) +xfs_qm_qoff_logitem_unpin (xfs_qoff_logitem_t *qf) {return ; }
@@ -536,8 +534,7 @@ static struct xfs_item_ops xfs_qm_qoffend_logitem_ops = { .iop_format = (void(*)(xfs_log_item_t*, xfs_log_iovec_t*)) xfs_qm_qoff_logitem_format, .iop_pin = (void(*)(xfs_log_item_t*))xfs_qm_qoff_logitem_pin, - .iop_unpin = (void (*)(xfs_log_item_t * ,int )) - xfs_qm_qoff_logitem_unpin, + .iop_unpin = (void(*)(xfs_log_item_t*))xfs_qm_qoff_logitem_unpin, .iop_unpin_remove = (void (*)(xfs_log_item_t *,xfs_trans_t *)) xfs_qm_qoff_logitem_unpin_remove, .iop_trylock = (uint(*)(xfs_log_item_t*))xfs_qm_qoff_logitem_trylock,
@@ -558,8 +555,7 @@ static struct xfs_item_ops xfs_qm_qoff_logitem_ops = { .iop_format = (void(*)(xfs_log_item_t*, xfs_log_iovec_t*)) xfs_qm_qoff_logitem_format, .iop_pin = (void(*)(xfs_log_item_t*))xfs_qm_qoff_logitem_pin, - .iop_unpin = (void (*)(xfs_log_item_t *,int )) - xfs_qm_qoff_logitem_unpin, + .iop_unpin = (void(*)(xfs_log_item_t*))xfs_qm_qoff_logitem_unpin, .iop_unpin_remove = (void (*)(xfs_log_item_t *,xfs_trans_t *)) xfs_qm_qoff_logitem_unpin_remove, .iop_trylock = (uint(*)(xfs_log_item_t*))xfs_qm_qoff_logitem_trylock,
fs/xfs/xfs_buf_item.c +22 -28
81 lines not on this page · raw message
fs/xfs/xfs_extfree_item.c +4 -4
@@ -106,7 +106,7 @@ xfs_efi_item_pin(xfs_efi_log_item_t *efip)*/ /*ARGSUSED*/ STATIC void -xfs_efi_item_unpin (xfs_efi_log_item_t *efip,int stale) +xfs_efi_item_unpin (xfs_efi_log_item_t *efip) {struct xfs_ail *ailp = efip->efi_item.li_ailp;
@@ -224,7 +224,7 @@ static struct xfs_item_ops xfs_efi_item_ops = { .iop_format = (void(*)(xfs_log_item_t*, xfs_log_iovec_t*)) xfs_efi_item_format, .iop_pin = (void(*)(xfs_log_item_t*))xfs_efi_item_pin, - .iop_unpin = (void(*)(xfs_log_item_t*, int))xfs_efi_item_unpin, + .iop_unpin = (void(*)(xfs_log_item_t*))xfs_efi_item_unpin, .iop_unpin_remove = (void (*)(xfs_log_item_t *,xfs_trans_t *)) xfs_efi_item_unpin_remove, .iop_trylock = (uint(*)(xfs_log_item_t*))xfs_efi_item_trylock,
@@ -425,7 +425,7 @@ xfs_efd_item_pin(xfs_efd_log_item_t *efdp)*/ /*ARGSUSED*/ STATIC void -xfs_efd_item_unpin (xfs_efd_log_item_t *efdp,int stale) +xfs_efd_item_unpin (xfs_efd_log_item_t *efdp) {return ; }
@@ -515,7 +515,7 @@ static struct xfs_item_ops xfs_efd_item_ops = { .iop_format = (void(*)(xfs_log_item_t*, xfs_log_iovec_t*)) xfs_efd_item_format, .iop_pin = (void(*)(xfs_log_item_t*))xfs_efd_item_pin, - .iop_unpin = (void(*)(xfs_log_item_t*, int))xfs_efd_item_unpin, + .iop_unpin = (void(*)(xfs_log_item_t*))xfs_efd_item_unpin, .iop_unpin_remove = (void (*)(xfs_log_item_t *,xfs_trans_t *)) xfs_efd_item_unpin_remove, .iop_trylock = (uint(*)(xfs_log_item_t*))xfs_efd_item_trylock,
fs/xfs/xfs_inode_item.c +3 -4
@@ -557,8 +557,7 @@ xfs_inode_item_pin(/* ARGSUSED */ STATIC voidxfs_inode_item_unpin ( -xfs_inode_log_item_t *iip, -int stale) +xfs_inode_log_item_t *iip) {struct xfs_inode *ip = iip->ili_inode;
@@ -574,7 +573,7 @@ xfs_inode_item_unpin_remove( xfs_inode_log_item_t *iip, xfs_trans_t *tp) { -xfs_inode_item_unpin (iip,0 ); +xfs_inode_item_unpin (iip); }/*
@@ -840,7 +839,7 @@ static struct xfs_item_ops xfs_inode_item_ops = { .iop_format = (void(*)(xfs_log_item_t*, xfs_log_iovec_t*)) xfs_inode_item_format, .iop_pin = (void(*)(xfs_log_item_t*))xfs_inode_item_pin, - .iop_unpin = (void(*)(xfs_log_item_t*, int))xfs_inode_item_unpin, + .iop_unpin = (void(*)(xfs_log_item_t*))xfs_inode_item_unpin, .iop_unpin_remove = (void (*)(xfs_log_item_t *,xfs_trans_t *)) xfs_inode_item_unpin_remove, .iop_trylock = (uint(*)(xfs_log_item_t*))xfs_inode_item_trylock,
fs/xfs/xfs_trans.c +1 -1
@@ -1451,6 +1451,6 @@ xfs_trans_chunk_committed(* flags, if anyone else stales the buffer we do not * want to pay any attention to it. */ -IOP_UNPIN (lip, lidp->lid_flags &XFS_LID_BUF_STALE ); +IOP_UNPIN (lip); } }
fs/xfs/xfs_trans.h +2 -3
@@ -159,7 +159,6 @@ typedef struct xfs_log_item_desc {#define XFS_LID_DIRTY 0x1#define XFS_LID_PINNED 0x2 -#define XFS_LID_BUF_STALE 0x8/* * This structure is used to maintain a chunk list of log_item_desc
@@ -833,7 +832,7 @@ typedef struct xfs_item_ops {uint (*iop_size)(xfs_log_item_t *);void (*iop_format)(xfs_log_item_t *,struct xfs_log_iovec *);void (*iop_pin)(xfs_log_item_t *); -void (*iop_unpin)(xfs_log_item_t *,int ); +void (*iop_unpin)(xfs_log_item_t *);void (*iop_unpin_remove)(xfs_log_item_t *,struct xfs_trans *);uint (*iop_trylock)(xfs_log_item_t *);void (*iop_unlock)(xfs_log_item_t *);
@@ -846,7 +845,7 @@ typedef struct xfs_item_ops {#define IOP_SIZE (ip) (*(ip)->li_ops->iop_size)(ip)#define IOP_FORMAT (ip,vp) (*(ip)->li_ops->iop_format)(ip, vp)#define IOP_PIN (ip) (*(ip)->li_ops->iop_pin)(ip) -#define IOP_UNPIN (ip, flags) (*(ip)->li_ops->iop_unpin)(ip, flags) +#define IOP_UNPIN (ip) (*(ip)->li_ops->iop_unpin)(ip)#define IOP_UNPIN_REMOVE (ip,tp) (*(ip)->li_ops->iop_unpin_remove)(ip, tp)#define IOP_TRYLOCK (ip) (*(ip)->li_ops->iop_trylock)(ip)#define IOP_UNLOCK (ip) (*(ip)->li_ops->iop_unlock)(ip)
fs/xfs/xfs_trans_buf.c +1 -2
@@ -696,7 +696,6 @@ xfs_trans_log_buf(xfs_trans_t *tp, tp->t_flags |=XFS_TRANS_DIRTY ; lidp->lid_flags |=XFS_LID_DIRTY ; - lidp->lid_flags &= ~XFS_LID_BUF_STALE ; bip->bli_flags |=XFS_BLI_LOGGED ;xfs_buf_item_log (bip, first, last); }
@@ -782,7 +781,7 @@ xfs_trans_binval( bip->bli_format.blf_flags |=XFS_BLI_CANCEL ;memset ((char *)(bip->bli_format.blf_data_map),0 , (bip->bli_format.blf_map_size *sizeof (uint))); - lidp->lid_flags |=XFS_LID_DIRTY |XFS_LID_BUF_STALE ; + lidp->lid_flags |=XFS_LID_DIRTY ; tp->t_flags |=XFS_TRANS_DIRTY ; }
Signature · 6 lines
1.6.5 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
PATCH 4/9 xfs: fix reservation release commit flag in xfs_bmap_add_attrfork()
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
xfs_bmap_add_attrfork() passes XFS_TRANS_PERM_LOG_RES to xfs_trans_commit() to indicate that the commit should release the permanent log reservation as part of the commit. This is wrong - the correct flag is XFS_TRANS_RELEASE_LOG_RES - and it is only by the chance that both these flags have the value of 0x4 that the code is doing the right thing.
Fix it by changing to use the correct flag.
--- fs/xfs/xfs_bmap.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
fs/xfs/xfs_bmap.c +1 -1
@@ -3829,7 +3829,7 @@ xfs_bmap_add_attrfork( }if ((error =xfs_bmap_finish (&tp, &flist, &committed))) goto error2; - error =xfs_trans_commit (tp,XFS_TRANS_PERM_LOG_RES ); + error =xfs_trans_commit (tp,XFS_TRANS_RELEASE_LOG_RES );ASSERT (ip->i_df.if_ext_max ==XFS_IFORK_DSIZE (ip) / (uint )sizeof (xfs_bmbt_rec_t));return error;
Signature · 6 lines
1.6.5 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
PATCH 5/9 xfs: split out iclog writing from xfs_trans_commit()
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
Split the the part of xfs_trans_commit() that deals with writing the transaction into the iclog into a separate function. This isolates the physical commit process from the logical commit operation and makes it easier to insert different transaction commit paths without affecting the existing algorithm adversely.
--- fs/xfs/xfs_trans.c | 387 +++++++++++++++++++++++++++------------------------- 1 files changed, 199 insertions(+), 188 deletions(-)
fs/xfs/xfs_trans.c +199 -188
@@ -47,8 +47,6 @@ STATIC void xfs_trans_apply_sb_deltas(xfs_trans_t *); -STATIC uint xfs_trans_count_vecs(xfs_trans_t *); -STATIC void xfs_trans_fill_vecs(xfs_trans_t *, xfs_log_iovec_t *); STATIC void xfs_trans_uncommit(xfs_trans_t *, uint); STATIC void xfs_trans_committed(xfs_trans_t *, int); STATIC void xfs_trans_chunk_committed(xfs_log_item_chunk_t *, xfs_lsn_t, int);
@@ -764,94 +762,126 @@ xfs_trans_unreserve_and_mod_sb( } } -/* -* xfs_trans_commit -* -* Commit the given transaction to the log a/synchronously. -* -* XFS disk error handling mechanism is not based on a typical -* transaction abort mechanism. Logically after the filesystem -* gets marked 'SHUTDOWN', we can't let any new transactions -* be durable - ie. committed to disk - because some metadata might -* be inconsistent. In such cases, this returns an error, and the -* caller may assume that all locked objects joined to the transaction -* have already been unlocked as if the commit had succeeded. -* Do not reference the transaction structure after this call. +* Total up the number of log iovecs needed to commit this +* transaction. The transaction itself needs one for the +* transaction header. Ask each dirty item in turn how many +* it needs to get the total. */ -/*ARGSUSED*/ -int -_xfs_trans_commit ( -xfs_trans_t *tp, -uint flags, -int *log_flushed) +static uint +xfs_trans_count_vecs ( +xfs_trans_t *tp) { -xfs_log_iovec_t *log_vector; -int nvec; -xfs_mount_t *mp; -xfs_lsn_t commit_lsn; -/* REFERENCED */ -int error; -int log_flags; -int sync; -#define XFS_TRANS_LOGVEC_COUNT 16 -xfs_log_iovec_t log_vector_fast[XFS_TRANS_LOGVEC_COUNT ]; -struct xlog_in_core *commit_iclog; -int shutdown; +int nvecs; +xfs_log_item_desc_t *lidp; - commit_lsn =-1 ; + nvecs =1 ; + lidp =xfs_trans_first_item (tp); +ASSERT (lidp !=NULL ); -/* -* Determine whether this commit is releasing a permanent -* log reservation or not. +/* In the non-debug case we need to start bailing out if we +* didn't find a log_item here, return zero and let trans_commit +* deal with it. */ -if (flags &XFS_TRANS_RELEASE_LOG_RES ) { -ASSERT (tp->t_flags &XFS_TRANS_PERM_LOG_RES ); - log_flags =XFS_LOG_REL_PERM_RESERV ; - }else { - log_flags =0 ; +if (lidp ==NULL ) +return 0 ; + +while (lidp !=NULL ) { +/* +* Skip items which aren't dirty in this transaction. +*/ +if (!(lidp->lid_flags &XFS_LID_DIRTY )) { + lidp =xfs_trans_next_item (tp, lidp); +continue ; + } + lidp->lid_size =IOP_SIZE (lidp->lid_item); + nvecs += lidp->lid_size; + lidp =xfs_trans_next_item (tp, lidp); } - mp = tp->t_mountp; + +return nvecs; +} + +/* +* Fill in the vector with pointers to data to be logged +* by this transaction. The transaction header takes +* the first vector, and then each dirty item takes the +* number of vectors it indicated it needed in xfs_trans_count_vecs(). +* +* As each item fills in the entries it needs, also pin the item +* so that it cannot be flushed out until the log write completes. +*/ +static void +xfs_trans_fill_vecs ( +struct xfs_trans *tp, +struct xfs_log_iovec *log_vector) +{ +xfs_log_item_desc_t *lidp; +struct xfs_log_iovec *vecp; +uint nitems;/* -* If there is nothing to be logged by the transaction, -* then unlock all of the items associated with the -* transaction and free the transaction structure. -* Also make sure to return any reserved blocks to -* the free pool. +* Skip over the entry for the transaction header, we'll +* fill that in at the end. */ -shut_us_down: - shutdown =XFS_FORCED_SHUTDOWN (mp) ?EIO :0 ; -if (!(tp->t_flags &XFS_TRANS_DIRTY ) || shutdown) { -xfs_trans_unreserve_and_mod_sb (tp); + vecp = log_vector +1 ; + + nitems =0 ; + lidp =xfs_trans_first_item (tp); +ASSERT (lidp); +while (lidp) { +/* Skip items which aren't dirty in this transaction. */ +if (!(lidp->lid_flags &XFS_LID_DIRTY )) { + lidp =xfs_trans_next_item (tp, lidp); +continue ; + } +/* -* It is indeed possible for the transaction to be -* not dirty but the dqinfo portion to be. All that -* means is that we have some (non-persistent) quota -* reservations that need to be unreserved. +* The item may be marked dirty but not log anything. This can +* be used to get called when a transaction is committed. */ -xfs_trans_unreserve_and_mod_dquots (tp); -if (tp->t_ticket) { - commit_lsn =xfs_log_done (mp, tp->t_ticket, -NULL , log_flags); -if (commit_lsn ==-1 && !shutdown) - shutdown =XFS_ERROR (EIO ); - } -current_restore_flags_nested (&tp->t_pflags,PF_FSTRANS ); -xfs_trans_free_items (tp, shutdown?XFS_TRANS_ABORT :0 ); -xfs_trans_free_busy (tp); -xfs_trans_free (tp); -XFS_STATS_INC (xs_trans_empty); -return (shutdown); +if (lidp->lid_size) + nitems++; +IOP_FORMAT (lidp->lid_item, vecp); + vecp += lidp->lid_size; +IOP_PIN (lidp->lid_item); + lidp =xfs_trans_next_item (tp, lidp); } -ASSERT (tp->t_ticket !=NULL );/* -* If we need to update the superblock, then do it now. +* Now that we've counted the number of items in this transaction, fill +* in the transaction header. Note that the transaction header does not +* have a log item. */ -if (tp->t_flags &XFS_TRANS_SB_DIRTY ) -xfs_trans_apply_sb_deltas (tp); -xfs_trans_apply_dquot_deltas (tp); + tp->t_header.th_magic =XFS_TRANS_HEADER_MAGIC ; + tp->t_header.th_type = tp->t_type; + tp->t_header.th_num_items = nitems; + log_vector->i_addr = (xfs_caddr_t)&tp->t_header; + log_vector->i_len =sizeof (xfs_trans_header_t); + log_vector->i_type =XLOG_REG_TYPE_TRANSHDR ; +} + +/* +* Format the transaction direct to the iclog. This isolates the physical +* transaction commit operation from the logical operation and hence allows +* other methods to be introduced without affecting the existing commit path. +*/ +static int +xfs_trans_commit_iclog ( +struct xfs_mount *mp, +struct xfs_trans *tp, +xfs_lsn_t *commit_lsn, +int flags) +{ +int shutdown; +int error; +int log_flags =0 ; +struct xlog_in_core *commit_iclog; +#define XFS_TRANS_LOGVEC_COUNT 16 +struct xfs_log_iovec log_vector_fast[XFS_TRANS_LOGVEC_COUNT ]; +struct xfs_log_iovec *log_vector; +uint nvec; +/* * Ask each log item how many log_vector entries it will
@@ -861,8 +891,7 @@ shut_us_down:*/ nvec =xfs_trans_count_vecs (tp);if (nvec ==0 ) { -xfs_force_shutdown (mp,SHUTDOWN_LOG_IO_ERROR ); - goto shut_us_down; +return ENOMEM ;/* triggers a shutdown! */ }else if (nvec <=XFS_TRANS_LOGVEC_COUNT ) { log_vector = log_vector_fast; }else {
@@ -877,6 +906,9 @@ shut_us_down:*/ xfs_trans_fill_vecs (tp, log_vector); +if (flags &XFS_TRANS_RELEASE_LOG_RES ) + log_flags =XFS_LOG_REL_PERM_RESERV ; + error =xfs_log_write (mp, log_vector, nvec, tp->t_ticket, &(tp->t_lsn));/*
@@ -884,18 +916,17 @@ shut_us_down:* at any time after this call. However, all the items associated * with the transaction are still locked and pinned in memory. */ - commit_lsn =xfs_log_done (mp, tp->t_ticket, &commit_iclog, log_flags); + *commit_lsn =xfs_log_done (mp, tp->t_ticket, &commit_iclog, log_flags); - tp->t_commit_lsn = commit_lsn; -if (nvec >XFS_TRANS_LOGVEC_COUNT ) { + tp->t_commit_lsn = *commit_lsn; +if (nvec >XFS_TRANS_LOGVEC_COUNT )kmem_free (log_vector); - }/* * If we got a log write error. Unpin the logitems that we * had pinned, clean up, free trans structure, and return error. */ -if (error || commit_lsn ==-1 ) { +if (error || *commit_lsn ==-1 ) {current_restore_flags_nested (&tp->t_pflags,PF_FSTRANS );xfs_trans_uncommit (tp, flags|XFS_TRANS_ABORT );return XFS_ERROR (EIO );
@@ -909,8 +940,6 @@ shut_us_down:*/ xfs_trans_unreserve_and_mod_sb (tp); - sync = tp->t_flags &XFS_TRANS_SYNC ; -/* * Tell the LM to call the transaction completion routine * when the log write with LSN commit_lsn completes (e.g.
@@ -953,7 +982,7 @@ shut_us_down:* the commit lsn of this transaction for dependency tracking * purposes. */ -xfs_trans_unlock_items (tp, commit_lsn); +xfs_trans_unlock_items (tp, *commit_lsn);/* * If we detected a log error earlier, finish committing
@@ -973,7 +1002,92 @@ shut_us_down:* and the items are released we can finally allow the iclog to * go to disk. */ - error =xfs_log_release_iclog (mp, commit_iclog); +return xfs_log_release_iclog (mp, commit_iclog); +} + + +/* +* xfs_trans_commit +* +* Commit the given transaction to the log a/synchronously. +* +* XFS disk error handling mechanism is not based on a typical +* transaction abort mechanism. Logically after the filesystem +* gets marked 'SHUTDOWN', we can't let any new transactions +* be durable - ie. committed to disk - because some metadata might +* be inconsistent. In such cases, this returns an error, and the +* caller may assume that all locked objects joined to the transaction +* have already been unlocked as if the commit had succeeded. +* Do not reference the transaction structure after this call. +*/ +/*ARGSUSED*/ +int +_xfs_trans_commit ( +xfs_trans_t *tp, +uint flags, +int *log_flushed) +{ +xfs_mount_t *mp = tp->t_mountp; +xfs_lsn_t commit_lsn =-1 ; +int error; +int log_flags =0 ; +int sync = tp->t_flags &XFS_TRANS_SYNC ; +int shutdown; + +/* +* Determine whether this commit is releasing a permanent +* log reservation or not. +*/ +if (flags &XFS_TRANS_RELEASE_LOG_RES ) { +ASSERT (tp->t_flags &XFS_TRANS_PERM_LOG_RES ); + log_flags =XFS_LOG_REL_PERM_RESERV ; + } + +/* +* If there is nothing to be logged by the transaction, +* then unlock all of the items associated with the +* transaction and free the transaction structure. +* Also make sure to return any reserved blocks to +* the free pool. +*/ +shut_us_down: + shutdown =XFS_FORCED_SHUTDOWN (mp) ?EIO :0 ; +if (!(tp->t_flags &XFS_TRANS_DIRTY ) || shutdown) { +xfs_trans_unreserve_and_mod_sb (tp); +/* +* It is indeed possible for the transaction to be +* not dirty but the dqinfo portion to be. All that +* means is that we have some (non-persistent) quota +* reservations that need to be unreserved. +*/ +xfs_trans_unreserve_and_mod_dquots (tp); +if (tp->t_ticket) { + commit_lsn =xfs_log_done (mp, tp->t_ticket, +NULL , log_flags); +if (commit_lsn ==-1 && !shutdown) + shutdown =XFS_ERROR (EIO ); + } +current_restore_flags_nested (&tp->t_pflags,PF_FSTRANS ); +xfs_trans_free_items (tp, shutdown?XFS_TRANS_ABORT :0 ); +xfs_trans_free_busy (tp); +xfs_trans_free (tp); +XFS_STATS_INC (xs_trans_empty); +return (shutdown); + } +ASSERT (tp->t_ticket !=NULL ); + +/* +* If we need to update the superblock, then do it now. +*/ +if (tp->t_flags &XFS_TRANS_SB_DIRTY ) +xfs_trans_apply_sb_deltas (tp); +xfs_trans_apply_dquot_deltas (tp); + + error =xfs_trans_commit_iclog (mp, tp, &commit_lsn, flags); +if (error ==ENOMEM ) { +xfs_force_shutdown (mp,SHUTDOWN_LOG_IO_ERROR ); + goto shut_us_down; + }/* * If the transaction needs to be synchronous, then force the
@@ -992,47 +1106,6 @@ shut_us_down:return (error); } - -/* -* Total up the number of log iovecs needed to commit this -* transaction. The transaction itself needs one for the -* transaction header. Ask each dirty item in turn how many -* it needs to get the total. -*/ -STATIC uint -xfs_trans_count_vecs ( -xfs_trans_t *tp) -{ -int nvecs; -xfs_log_item_desc_t *lidp; - - nvecs =1 ; - lidp =xfs_trans_first_item (tp); -ASSERT (lidp !=NULL ); - -/* In the non-debug case we need to start bailing out if we -* didn't find a log_item here, return zero and let trans_commit -* deal with it. -*/ -if (lidp ==NULL ) -return 0 ; - -while (lidp !=NULL ) { -/* -* Skip items which aren't dirty in this transaction. -*/ -if (!(lidp->lid_flags &XFS_LID_DIRTY )) { - lidp =xfs_trans_next_item (tp, lidp); -continue ; - } - lidp->lid_size =IOP_SIZE (lidp->lid_item); - nvecs += lidp->lid_size; - lidp =xfs_trans_next_item (tp, lidp); - } - -return nvecs; -} -/* * Called from the trans_commit code when we notice that * the filesystem is in the middle of a forced shutdown.
@@ -1063,68 +1136,6 @@ xfs_trans_uncommit( }/* -* Fill in the vector with pointers to data to be logged -* by this transaction. The transaction header takes -* the first vector, and then each dirty item takes the -* number of vectors it indicated it needed in xfs_trans_count_vecs(). -* -* As each item fills in the entries it needs, also pin the item -* so that it cannot be flushed out until the log write completes. -*/ -STATIC void -xfs_trans_fill_vecs ( -xfs_trans_t *tp, -xfs_log_iovec_t *log_vector) -{ -xfs_log_item_desc_t *lidp; -xfs_log_iovec_t *vecp; -uint nitems; - -/* -* Skip over the entry for the transaction header, we'll -* fill that in at the end. -*/ - vecp = log_vector +1 ;/* pointer arithmetic */ - - nitems =0 ; - lidp =xfs_trans_first_item (tp); -ASSERT (lidp !=NULL ); -while (lidp !=NULL ) { -/* -* Skip items which aren't dirty in this transaction. -*/ -if (!(lidp->lid_flags &XFS_LID_DIRTY )) { - lidp =xfs_trans_next_item (tp, lidp); -continue ; - } -/* -* The item may be marked dirty but not log anything. -* This can be used to get called when a transaction -* is committed. -*/ -if (lidp->lid_size) { - nitems++; - } -IOP_FORMAT (lidp->lid_item, vecp); - vecp += lidp->lid_size;/* pointer arithmetic */ -IOP_PIN (lidp->lid_item); - lidp =xfs_trans_next_item (tp, lidp); - } - -/* -* Now that we've counted the number of items in this -* transaction, fill in the transaction header. -*/ - tp->t_header.th_magic =XFS_TRANS_HEADER_MAGIC ; - tp->t_header.th_type = tp->t_type; - tp->t_header.th_num_items = nitems; - log_vector->i_addr = (xfs_caddr_t)&tp->t_header; - log_vector->i_len =sizeof (xfs_trans_header_t); - log_vector->i_type =XLOG_REG_TYPE_TRANSHDR ; -} - - -/* * Unlock all of the transaction's items and free the transaction. * The transaction must not have modified any of its items, because * there is no way to restore them to their previous state.
Signature · 6 lines
1.6.5 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
PATCH 6/9 xfs: clean up xfs_trans_commit logic even more
In reply to Dave ChinnerFrom: Christoph Hellwig <[email protected]>
Review
Quoted 5 lines
(quoted diff) +5
-0+shut_us_down: + shutdown = XFS_FORCED_SHUTDOWN(mp) ? EIO : 0; + if (!(tp->t_flags & XFS_TRANS_DIRTY) || shutdown) { + xfs_trans_unreserve_and_mod_sb(tp); + /*
This whole area in _xfs_trans_commit is still a complete mess.
So while touching this code, unravel this mess as well to make the whole flow of the function simpler and clearer.
--- fs/xfs/xfs_trans.c | 69 ++++++++++++++++++++++++++++------------------------ 1 files changed, 37 insertions(+), 32 deletions(-)
fs/xfs/xfs_trans.c +37 -32
@@ -1020,19 +1020,17 @@ xfs_trans_commit_iclog(* have already been unlocked as if the commit had succeeded. * Do not reference the transaction structure after this call. */ -/*ARGSUSED*/ int _xfs_trans_commit ( -xfs_trans_t *tp, -uint flags, -int *log_flushed) +struct xfs_trans *tp, +uint flags, +int *log_flushed) { -xfs_mount_t *mp = tp->t_mountp; +struct xfs_mount *mp = tp->t_mountp;xfs_lsn_t commit_lsn =-1 ; -int error; +int error =0 ;int log_flags =0 ;int sync = tp->t_flags &XFS_TRANS_SYNC ; -int shutdown;/* * Determine whether this commit is releasing a permanent
@@ -1050,30 +1048,14 @@ _xfs_trans_commit(* Also make sure to return any reserved blocks to * the free pool. */ -shut_us_down: - shutdown =XFS_FORCED_SHUTDOWN (mp) ?EIO :0 ; -if (!(tp->t_flags &XFS_TRANS_DIRTY ) || shutdown) { -xfs_trans_unreserve_and_mod_sb (tp); -/* -* It is indeed possible for the transaction to be -* not dirty but the dqinfo portion to be. All that -* means is that we have some (non-persistent) quota -* reservations that need to be unreserved. -*/ -xfs_trans_unreserve_and_mod_dquots (tp); -if (tp->t_ticket) { - commit_lsn =xfs_log_done (mp, tp->t_ticket, -NULL , log_flags); -if (commit_lsn ==-1 && !shutdown) - shutdown =XFS_ERROR (EIO ); - } -current_restore_flags_nested (&tp->t_pflags,PF_FSTRANS ); -xfs_trans_free_items (tp, shutdown?XFS_TRANS_ABORT :0 ); -xfs_trans_free_busy (tp); -xfs_trans_free (tp); -XFS_STATS_INC (xs_trans_empty); -return (shutdown); +if (!(tp->t_flags &XFS_TRANS_DIRTY )) + goto out_unreserve; + +if (XFS_FORCED_SHUTDOWN (mp)) { + error =XFS_ERROR (EIO ); + goto out_unreserve; } +ASSERT (tp->t_ticket !=NULL );/*
@@ -1086,7 +1068,8 @@ shut_us_down: error =xfs_trans_commit_iclog (mp, tp, &commit_lsn, flags);if (error ==ENOMEM ) {xfs_force_shutdown (mp,SHUTDOWN_LOG_IO_ERROR ); - goto shut_us_down; + error =XFS_ERROR (EIO ); + goto out_unreserve; }/*
@@ -1103,7 +1086,29 @@ shut_us_down:XFS_STATS_INC (xs_trans_async); } -return (error); +return error; + +out_unreserve: +xfs_trans_unreserve_and_mod_sb (tp); + +/* +* It is indeed possible for the transaction to be not dirty but +* the dqinfo portion to be. All that means is that we have some +* (non-persistent) quota reservations that need to be unreserved. +*/ +xfs_trans_unreserve_and_mod_dquots (tp); +if (tp->t_ticket) { + commit_lsn =xfs_log_done (mp, tp->t_ticket,NULL , log_flags); +if (commit_lsn ==-1 && !error) + error =XFS_ERROR (EIO ); + } +current_restore_flags_nested (&tp->t_pflags,PF_FSTRANS ); +xfs_trans_free_items (tp, error ?XFS_TRANS_ABORT :0 ); +xfs_trans_free_busy (tp); +xfs_trans_free (tp); + +XFS_STATS_INC (xs_trans_empty); +return error; }/*
Signature · 6 lines
1.6.5 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
PATCH 7/9 xfs: update and factor xfs_trans_committed()
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
The function header to xfs-trans_committed has long had this comment:
* THIS SHOULD BE REWRITTEN TO USE xfs_trans_next_item()
To prepare for different methods of committing items, convert the code to use xfs_trans_next_item() and factor the code into smaller, more digestible chunks.
--- fs/xfs/xfs_trans.c | 224 ++++++++++++++++++++++------------------------------ 1 files changed, 95 insertions(+), 129 deletions(-)
fs/xfs/xfs_trans.c +95 -129
@@ -49,7 +49,6 @@ STATIC void xfs_trans_apply_sb_deltas(xfs_trans_t *); STATIC void xfs_trans_uncommit(xfs_trans_t *, uint); STATIC void xfs_trans_committed(xfs_trans_t *, int); -STATIC void xfs_trans_chunk_committed(xfs_log_item_chunk_t *, xfs_lsn_t, int); STATIC void xfs_trans_free(xfs_trans_t *);kmem_zone_t *xfs_trans_zone;
@@ -1301,60 +1300,86 @@ xfs_trans_roll( }/* -* THIS SHOULD BE REWRITTEN TO USE xfs_trans_next_item(). +* The committed item processing consists of calling the committed routine of +* each logged item, updating the item's position in the AIL if necessary, and +* unpinning each item. If the committed routine returns -1, then do nothing +* further with the item because it may have been freed. * -* This is typically called by the LM when a transaction has been fully -* committed to disk. It needs to unpin the items which have -* been logged by the transaction and update their positions -* in the AIL if necessary. -* This also gets called when the transactions didn't get written out -* because of an I/O error. Abortflag & XFS_LI_ABORTED is set then. +* Since items are unlocked when they are copied to the incore log, it is +* possible for two transactions to be completing and manipulating the same +* item simultaneously. The AIL lock will protect the lsn field of each item. +* The value of this field can never go backwards. * -* Call xfs_trans_chunk_committed() to process the items in -* each chunk. +* We unpin the items after repositioning them in the AIL, because otherwise +* they could be immediately flushed and we'd have to race with the flusher +* trying to pull the item from the AIL as we add it. */ -STATIC void -xfs_trans_committed ( -xfs_trans_t *tp, -int abortflag) +static void +xfs_trans_item_committed ( +xfs_log_item_t *lip, +xfs_lsn_t commit_lsn, +int aborted) { -xfs_log_item_chunk_t *licp; -xfs_log_item_chunk_t *next_licp; -xfs_log_busy_chunk_t *lbcp; -xfs_log_busy_slot_t *lbsp; -int i; +xfs_lsn_t item_lsn; +struct xfs_ail *ailp; + +if (aborted) + lip->li_flags |=XFS_LI_ABORTED ;/* -* Call the transaction's completion callback if there -* is one. +* Send in the ABORTED flag to the COMMITTED routine so that it knows +* whether the transaction was aborted or not. */ -if (tp->t_callback !=NULL ) { - tp->t_callback (tp, tp->t_callarg); - } + item_lsn =IOP_COMMITTED (lip, commit_lsn);/* -* Special case the chunk embedded in the transaction. +* If the committed routine returns -1, item has been freed. */ - licp = &(tp->t_items); -if (!(xfs_lic_are_all_free (licp))) { -xfs_trans_chunk_committed (licp, tp->t_lsn, abortflag); - } +if (XFS_LSN_CMP (item_lsn, (xfs_lsn_t)-1 ) ==0 ) +return ;/* -* Process the items in each chunk in turn. +* If the returned lsn is greater than what it contained before, update +* the location of the item in the AIL. If it is not, then do nothing. +* Items can never move backwards in the AIL. +* +* While the new lsn should usually be greater, it is possible that a +* later transaction completing simultaneously with an earlier one +* using the same item could complete first with a higher lsn. This +* would cause the earlier transaction to fail the test below. */ - licp = licp->lic_next; -while (licp !=NULL ) { -ASSERT (!xfs_lic_are_all_free (licp)); -xfs_trans_chunk_committed (licp, tp->t_lsn, abortflag); - next_licp = licp->lic_next; -kmem_free (licp); - licp = next_licp; + ailp = lip->li_ailp; +spin_lock (&ailp->xa_lock); +if (XFS_LSN_CMP (item_lsn, lip->li_lsn) >0 ) { +/* +* This will set the item's lsn to item_lsn and update the +* position of the item in the AIL. +* +* xfs_trans_ail_update() drops the AIL lock. +*/ +xfs_trans_ail_update (ailp, lip, item_lsn); + }else { +spin_unlock (&ailp->xa_lock); }/* -* Clear all the per-AG busy list items listed in this transaction +* Now that we've repositioned the item in the AIL, unpin it so it can +* be flushed. Pass information about buffer stale state down from the +* log item flags, if anyone else stales the buffer we do not want to +* pay any attention to it. */ +IOP_UNPIN (lip); +} + +/* Clear all the per-AG busy list items listed in this transaction */ +static void +xfs_trans_clear_busy_extents ( +struct xfs_trans *tp) +{ +xfs_log_busy_chunk_t *lbcp; +xfs_log_busy_slot_t *lbsp; +int i; + lbcp = &tp->t_busy;while (lbcp !=NULL ) {for (i =0 , lbsp = lbcp->lbc_busy; i < lbcp->lbc_unused; i++, lbsp++) {
@@ -1366,107 +1391,48 @@ xfs_trans_committed( lbcp = lbcp->lbc_next; }xfs_trans_free_busy (tp); - -/* -* That's it for the transaction structure. Free it. -*/ -xfs_trans_free (tp); }/* -* This is called to perform the commit processing for each -* item described by the given chunk. -* -* The commit processing consists of unlocking items which were -* held locked with the SYNC_UNLOCK attribute, calling the committed -* routine of each logged item, updating the item's position in the AIL -* if necessary, and unpinning each item. If the committed routine -* returns -1, then do nothing further with the item because it -* may have been freed. -* -* Since items are unlocked when they are copied to the incore -* log, it is possible for two transactions to be completing -* and manipulating the same item simultaneously. The AIL lock -* will protect the lsn field of each item. The value of this -* field can never go backwards. +* This is typically called by the LM when a transaction has been fully +* committed to disk. It needs to unpin the items which have +* been logged by the transaction and update their positions +* in the AIL if necessary. * -* We unpin the items after repositioning them in the AIL, because -* otherwise they could be immediately flushed and we'd have to race -* with the flusher trying to pull the item from the AIL as we add it. +* This also gets called when the transactions didn't get written out +* because of an I/O error. Abortflag & XFS_LI_ABORTED is set then. */ STATIC void -xfs_trans_chunk_committed ( -xfs_log_item_chunk_t *licp, -xfs_lsn_t lsn, -int aborted) +xfs_trans_committed ( +xfs_trans_t *tp, +int abortflag) {xfs_log_item_desc_t *lidp; -xfs_log_item_t *lip; -xfs_lsn_t item_lsn; -int i; - - lidp = licp->lic_descs; -for (i =0 ; i < licp->lic_unused; i++, lidp++) { -struct xfs_ail *ailp; - -if (xfs_lic_isfree (licp, i)) { -continue ; - } - - lip = lidp->lid_item; -if (aborted) - lip->li_flags |=XFS_LI_ABORTED ; - -/* -* Send in the ABORTED flag to the COMMITTED routine -* so that it knows whether the transaction was aborted -* or not. -*/ - item_lsn =IOP_COMMITTED (lip, lsn); +xfs_log_item_chunk_t *licp; +xfs_log_item_chunk_t *next_licp; -/* -* If the committed routine returns -1, make -* no more references to the item. -*/ -if (XFS_LSN_CMP (item_lsn, (xfs_lsn_t)-1 ) ==0 ) { -continue ; - } +/* +* Call the transaction's completion callback if there +* is one. +*/ +if (tp->t_callback !=NULL ) { + tp->t_callback (tp, tp->t_callarg); + } -/* -* If the returned lsn is greater than what it -* contained before, update the location of the -* item in the AIL. If it is not, then do nothing. -* Items can never move backwards in the AIL. -* -* While the new lsn should usually be greater, it -* is possible that a later transaction completing -* simultaneously with an earlier one using the -* same item could complete first with a higher lsn. -* This would cause the earlier transaction to fail -* the test below. -*/ - ailp = lip->li_ailp; -spin_lock (&ailp->xa_lock); -if (XFS_LSN_CMP (item_lsn, lip->li_lsn) >0 ) { -/* -* This will set the item's lsn to item_lsn -* and update the position of the item in -* the AIL. -* -* xfs_trans_ail_update() drops the AIL lock. -*/ -xfs_trans_ail_update (ailp, lip, item_lsn); - }else { -spin_unlock (&ailp->xa_lock); - } +for (lidp =xfs_trans_first_item (tp); + lidp !=NULL ; + lidp =xfs_trans_next_item (tp, lidp)) { +xfs_trans_item_committed (lidp->lid_item, tp->t_lsn, abortflag); + } -/* -* Now that we've repositioned the item in the AIL, -* unpin it so it can be flushed. Pass information -* about buffer stale state down from the log item -* flags, if anyone else stales the buffer we do not -* want to pay any attention to it. -*/ -IOP_UNPIN (lip); +/* free the item chunks, ignoring the embedded chunk */ + licp = tp->t_items.lic_next; +while (licp !=NULL ) { + next_licp = licp->lic_next; +kmem_free (licp); + licp = next_licp; } + +xfs_trans_clear_busy_extents (tp); +xfs_trans_free (tp); }
Signature · 6 lines
1.6.5 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
PATCH 8/9 xfs: Clean up xfs_trans_committed code after factoring
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
Now that the code has been factored, clean up all the remaining style cruft, simplify the code and re-order functions so that it doesn't need forward declarations.
Also move the remaining functions that require forward declarations (xfs_trans_uncommit, xfs_trans_free) so that all the forward declarations can be removed from the file.
--- fs/xfs/xfs_trans.c | 356 ++++++++++++++++++++++++---------------------------- 1 files changed, 166 insertions(+), 190 deletions(-)
fs/xfs/xfs_trans.c +166 -190
@@ -45,20 +45,12 @@#include "xfs_trans_space.h" #include "xfs_inode_item.h" - -STATIC void xfs_trans_apply_sb_deltas(xfs_trans_t *); -STATIC void xfs_trans_uncommit(xfs_trans_t *, uint); -STATIC void xfs_trans_committed(xfs_trans_t *, int); -STATIC void xfs_trans_free(xfs_trans_t *); -kmem_zone_t *xfs_trans_zone; -/* * Reservation functions here avoid a huge stack in xfs_trans_init * due to register overflow from temporaries in the calculations. */ - STATIC uintxfs_calc_write_reservation (xfs_mount_t *mp) {
@@ -258,6 +250,19 @@ _xfs_trans_alloc( }/* +* Free the transaction structure. If there is more clean up +* to do when the structure is freed, add it here. +*/ +STATIC void +xfs_trans_free ( +xfs_trans_t *tp) +{ +atomic_dec (&tp->t_mountp->m_active_trans); +xfs_trans_free_dqinfo (tp); +kmem_zone_free (xfs_trans_zone, tp); +} + +/* * This is called to create a new transaction which will share the * permanent log reservation of the given transaction. The remaining * unused block and rt extent reservations are also inherited. This
@@ -769,7 +774,7 @@ xfs_trans_unreserve_and_mod_sb(*/ static uint xfs_trans_count_vecs ( -xfs_trans_t *tp) +struct xfs_trans *tp) {int nvecs;xfs_log_item_desc_t *lidp;
@@ -861,6 +866,158 @@ xfs_trans_fill_vecs( }/* +* The committed item processing consists of calling the committed routine of +* each logged item, updating the item's position in the AIL if necessary, and +* unpinning each item. If the committed routine returns -1, then do nothing +* further with the item because it may have been freed. +* +* Since items are unlocked when they are copied to the incore log, it is +* possible for two transactions to be completing and manipulating the same +* item simultaneously. The AIL lock will protect the lsn field of each item. +* The value of this field can never go backwards. +* +* We unpin the items after repositioning them in the AIL, because otherwise +* they could be immediately flushed and we'd have to race with the flusher +* trying to pull the item from the AIL as we add it. +*/ +static void +xfs_trans_item_committed ( +struct xfs_log_item *lip, +xfs_lsn_t commit_lsn, +int aborted) +{ +xfs_lsn_t item_lsn; +struct xfs_ail *ailp; + +if (aborted) + lip->li_flags |=XFS_LI_ABORTED ; + item_lsn =IOP_COMMITTED (lip, commit_lsn); + +/* If the committed routine returns -1, item has been freed. */ +if (XFS_LSN_CMP (item_lsn, (xfs_lsn_t)-1 ) ==0 ) +return ; + +/* +* If the returned lsn is greater than what it contained before, update +* the location of the item in the AIL. If it is not, then do nothing. +* Items can never move backwards in the AIL. +* +* While the new lsn should usually be greater, it is possible that a +* later transaction completing simultaneously with an earlier one +* using the same item could complete first with a higher lsn. This +* would cause the earlier transaction to fail the test below. +*/ + ailp = lip->li_ailp; +spin_lock (&ailp->xa_lock); +if (XFS_LSN_CMP (item_lsn, lip->li_lsn) >0 ) { +/* +* This will set the item's lsn to item_lsn and update the +* position of the item in the AIL. +* +* xfs_trans_ail_update() drops the AIL lock. +*/ +xfs_trans_ail_update (ailp, lip, item_lsn); + }else { +spin_unlock (&ailp->xa_lock); + } + +/* +* Now that we've repositioned the item in the AIL, unpin it so it can +* be flushed. Pass information about buffer stale state down from the +* log item flags, if anyone else stales the buffer we do not want to +* pay any attention to it. +*/ +IOP_UNPIN (lip); +} + +/* Clear all the per-AG busy list items listed in this transaction */ +static void +xfs_trans_clear_busy_extents ( +struct xfs_trans *tp) +{ +xfs_log_busy_chunk_t *lbcp; +xfs_log_busy_slot_t *lbsp; +int i; + +for (lbcp = &tp->t_busy; lbcp !=NULL ; lbcp = lbcp->lbc_next) { + i =0 ; +for (lbsp = lbcp->lbc_busy; i < lbcp->lbc_unused; i++, lbsp++) { +if (XFS_LBC_ISFREE (lbcp, i)) +continue ; +xfs_alloc_clear_busy (tp, lbsp->lbc_ag, lbsp->lbc_idx); + } + } +xfs_trans_free_busy (tp); +} + +/* +* This is typically called by the LM when a transaction has been fully +* committed to disk. It needs to unpin the items which have +* been logged by the transaction and update their positions +* in the AIL if necessary. +* +* This also gets called when the transactions didn't get written out +* because of an I/O error. Abortflag & XFS_LI_ABORTED is set then. +*/ +STATIC void +xfs_trans_committed ( +struct xfs_trans *tp, +int abortflag) +{ +xfs_log_item_desc_t *lidp; +xfs_log_item_chunk_t *licp; +xfs_log_item_chunk_t *next_licp; + +/* Call the transaction's completion callback if there is one. */ +if (tp->t_callback !=NULL ) + tp->t_callback (tp, tp->t_callarg); + +for (lidp =xfs_trans_first_item (tp); + lidp !=NULL ; + lidp =xfs_trans_next_item (tp, lidp)) { +xfs_trans_item_committed (lidp->lid_item, tp->t_lsn, abortflag); + } + +/* free the item chunks, ignoring the embedded chunk */ +for (licp = tp->t_items.lic_next; licp !=NULL ; licp = next_licp) { + next_licp = licp->lic_next; +kmem_free (licp); + } + +xfs_trans_clear_busy_extents (tp); +xfs_trans_free (tp); +} + +/* +* Called from the trans_commit code when we notice that +* the filesystem is in the middle of a forced shutdown. +*/ +STATIC void +xfs_trans_uncommit ( +struct xfs_trans *tp, +uint flags) +{ +xfs_log_item_desc_t *lidp; + +for (lidp =xfs_trans_first_item (tp); + lidp !=NULL ; + lidp =xfs_trans_next_item (tp, lidp)) { +/* +* Unpin all but those that aren't dirty. +*/ +if (lidp->lid_flags &XFS_LID_DIRTY ) +IOP_UNPIN_REMOVE (lidp->lid_item, tp); + } + +xfs_trans_unreserve_and_mod_sb (tp); +xfs_trans_unreserve_and_mod_dquots (tp); + +xfs_trans_free_items (tp, flags); +xfs_trans_free_busy (tp); +xfs_trans_free (tp); +} + +/* * Format the transaction direct to the iclog. This isolates the physical * transaction commit operation from the logical operation and hence allows * other methods to be introduced without affecting the existing commit path.
@@ -1111,35 +1268,6 @@ out_unreserve: }/* -* Called from the trans_commit code when we notice that -* the filesystem is in the middle of a forced shutdown. -*/ -STATIC void -xfs_trans_uncommit ( -xfs_trans_t *tp, -uint flags) -{ -xfs_log_item_desc_t *lidp; - -for (lidp =xfs_trans_first_item (tp); - lidp !=NULL ; - lidp =xfs_trans_next_item (tp, lidp)) { -/* -* Unpin all but those that aren't dirty. -*/ -if (lidp->lid_flags &XFS_LID_DIRTY ) -IOP_UNPIN_REMOVE (lidp->lid_item, tp); - } - -xfs_trans_unreserve_and_mod_sb (tp); -xfs_trans_unreserve_and_mod_dquots (tp); - -xfs_trans_free_items (tp, flags); -xfs_trans_free_busy (tp); -xfs_trans_free (tp); -} - -/* * Unlock all of the transaction's items and free the transaction. * The transaction must not have modified any of its items, because * there is no way to restore them to their previous state.
@@ -1215,20 +1343,6 @@ xfs_trans_cancel(xfs_trans_free (tp); } - -/* -* Free the transaction structure. If there is more clean up -* to do when the structure is freed, add it here. -*/ -STATIC void -xfs_trans_free ( -xfs_trans_t *tp) -{ -atomic_dec (&tp->t_mountp->m_active_trans); -xfs_trans_free_dqinfo (tp); -kmem_zone_free (xfs_trans_zone, tp); -} -/* * Roll from one trans in the sequence of PERMANENT transactions to * the next: permanent transactions are only flushed out when
@@ -1298,141 +1412,3 @@ xfs_trans_roll(xfs_trans_ihold (trans, dp);return 0 ; } - -/* -* The committed item processing consists of calling the committed routine of -* each logged item, updating the item's position in the AIL if necessary, and -* unpinning each item. If the committed routine returns -1, then do nothing -* further with the item because it may have been freed. -* -* Since items are unlocked when they are copied to the incore log, it is -* possible for two transactions to be completing and manipulating the same -* item simultaneously. The AIL lock will protect the lsn field of each item. -* The value of this field can never go backwards. -* -* We unpin the items after repositioning them in the AIL, because otherwise -* they could be immediately flushed and we'd have to race with the flusher -* trying to pull the item from the AIL as we add it. -*/ -static void -xfs_trans_item_committed ( -xfs_log_item_t *lip, -xfs_lsn_t commit_lsn, -int aborted) -{ -xfs_lsn_t item_lsn; -struct xfs_ail *ailp; - -if (aborted) - lip->li_flags |=XFS_LI_ABORTED ; - -/* -* Send in the ABORTED flag to the COMMITTED routine so that it knows -* whether the transaction was aborted or not. -*/ - item_lsn =IOP_COMMITTED (lip, commit_lsn); - -/* -* If the committed routine returns -1, item has been freed. -*/ -if (XFS_LSN_CMP (item_lsn, (xfs_lsn_t)-1 ) ==0 ) -return ; - -/* -* If the returned lsn is greater than what it contained before, update -* the location of the item in the AIL. If it is not, then do nothing. -* Items can never move backwards in the AIL. -* -* While the new lsn should usually be greater, it is possible that a -* later transaction completing simultaneously with an earlier one -* using the same item could complete first with a higher lsn. This -* would cause the earlier transaction to fail the test below. -*/ - ailp = lip->li_ailp; -spin_lock (&ailp->xa_lock); -if (XFS_LSN_CMP (item_lsn, lip->li_lsn) >0 ) { -/* -* This will set the item's lsn to item_lsn and update the -* position of the item in the AIL. -* -* xfs_trans_ail_update() drops the AIL lock. -*/ -xfs_trans_ail_update (ailp, lip, item_lsn); - }else { -spin_unlock (&ailp->xa_lock); - } - -/* -* Now that we've repositioned the item in the AIL, unpin it so it can -* be flushed. Pass information about buffer stale state down from the -* log item flags, if anyone else stales the buffer we do not want to -* pay any attention to it. -*/ -IOP_UNPIN (lip); -} - -/* Clear all the per-AG busy list items listed in this transaction */ -static void -xfs_trans_clear_busy_extents ( -struct xfs_trans *tp) -{ -xfs_log_busy_chunk_t *lbcp; -xfs_log_busy_slot_t *lbsp; -int i; - - lbcp = &tp->t_busy; -while (lbcp !=NULL ) { -for (i =0 , lbsp = lbcp->lbc_busy; i < lbcp->lbc_unused; i++, lbsp++) { -if (!XFS_LBC_ISFREE (lbcp, i)) { -xfs_alloc_clear_busy (tp, lbsp->lbc_ag, - lbsp->lbc_idx); - } - } - lbcp = lbcp->lbc_next; - } -xfs_trans_free_busy (tp); -} - -/* -* This is typically called by the LM when a transaction has been fully -* committed to disk. It needs to unpin the items which have -* been logged by the transaction and update their positions -* in the AIL if necessary. -* -* This also gets called when the transactions didn't get written out -* because of an I/O error. Abortflag & XFS_LI_ABORTED is set then. -*/ -STATIC void -xfs_trans_committed ( -xfs_trans_t *tp, -int abortflag) -{ -xfs_log_item_desc_t *lidp; -xfs_log_item_chunk_t *licp; -xfs_log_item_chunk_t *next_licp; - -/* -* Call the transaction's completion callback if there -* is one. -*/ -if (tp->t_callback !=NULL ) { - tp->t_callback (tp, tp->t_callarg); - } - -for (lidp =xfs_trans_first_item (tp); - lidp !=NULL ; - lidp =xfs_trans_next_item (tp, lidp)) { -xfs_trans_item_committed (lidp->lid_item, tp->t_lsn, abortflag); - } - -/* free the item chunks, ignoring the embedded chunk */ - licp = tp->t_items.lic_next; -while (licp !=NULL ) { - next_licp = licp->lic_next; -kmem_free (licp); - licp = next_licp; - } - -xfs_trans_clear_busy_extents (tp); -xfs_trans_free (tp); -}
Signature · 6 lines
1.6.5 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 8/9] xfs: Clean up xfs_trans_committed code after factoring
Quoted 9 lines · Dave Chinner
From: Dave Chinner <[email protected]>
Now that the code has been factored, clean up all the remaining style cruft, simplify the code and re-order functions so that it doesn't need forward declarations.
Also move the remaining functions that require forward declarations (
xfs_trans_uncommit,xfs_trans_free) so that all the forward declarations can be removed from the file.
Looks good,
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 9/9 xfs: log ticket reservation underestimates the number of iclogs
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
When allocation a ticket for a transaction, the ticket is initialised with the worst case log space usage based on the number of bytes the transaction may consume. Part of this calculation is the number of log headers required for the iclog space used up by the transaction.
This calculation makes an undocumented assumption that if the transaction uses the log header space reservation on an iclog, then it consumes either the entire iclog or it completes. That is - the transaction that is first in an iclog is the transaction that the log header reservation is accounted to. If the transaction is larger than the iclog, then it will use the entire iclog itself. Document this assumption.
Further, the current calculation uses the rule that we can fit iclog_size bytes of transaction data into an iclog. This is in correct - the amount of space available in an iclog for transaction data is the size of the iclog minus the space used for log record headers. This means that the calculation is out by 512 bytes per 32k of log space the transaction can consume. This is rarely an issue because maximally sized transactions are extremely uncommon, and for 4k block size filesystems maximal transaction reservations are about 400kb. Hence the error in this case is less than the size of an iclog, so that makes it even harder to hit.
However, anyone using larger directory blocks (16k directory blocks push the maximum transaction size to approx. 900k on a 4k block size filesystem) or larger block size (e.g. 64k blocks push transactions to the 3-4MB size) could see the error grow to more than an iclog and at this point the transaction is guaranteed to get a reservation underrun and shutdown the filesystem.
Fix this by adjusting the calculation to calculate the correct number of iclogs required and account for them all up front.
--- fs/xfs/xfs_log.c | 55 +++++++++++++++++++++++++++++++++++++++++------------ 1 files changed, 42 insertions(+), 13 deletions(-)
fs/xfs/xfs_log.c +42 -13
@@ -664,7 +664,10 @@ xfs_log_item_init(/* * Write region vectors to log. The write happens using the space reservation * of the ticket (tic). It is not a requirement that all writes for a given -* transaction occur with one call to xfs_log_write(). +* transaction occur with one call to xfs_log_write(). However, it is important +* to note that the transaction reservation code makes an assumption about the +* number of log headers a transaction requires that may be violated if you +* don't pass all the transaction vectors in one call.... */ int xfs_log_write(
@@ -3156,14 +3159,16 @@ xfs_log_ticket_get(* Allocate and initialise a new log ticket. */ STATIC xlog_ticket_t * -xlog_ticket_alloc (xlog_t *log, -int unit_bytes, -int cnt, -char client, -uint xflags) +xlog_ticket_alloc ( +struct log *log, +int unit_bytes, +int cnt, +char client, +uint xflags) { -xlog_ticket_t *tic; +struct xlog_ticket *tic;uint num_headers; +int iclog_space; tic =kmem_zone_zalloc (xfs_log_ticket_zone,KM_SLEEP |KM_MAYFAIL );if (!tic)
@@ -3207,16 +3212,40 @@ xlog_ticket_alloc(xlog_t *log,/* for start-rec */ unit_bytes +=sizeof (xlog_op_header_t); -/* for LR headers */ - num_headers = ((unit_bytes + log->l_iclog_size-1 ) >> log->l_iclog_size_log); +/* +* for LR headers - the space for data in an iclog is the size minus +* the space used for the headers. If we use the iclog size, then we +* undercalculate the number of headers required. +* +* Furthermore - the addition of op headers for split-recs might +* increase the space required enough to require more log and op +* headers, so take that into account too. +* +* IMPORTANT: This reservation makes the assumption that if this +* transaction is the first in an iclog and hence has the LR headers +* accounted to it, then the remaining space in the iclog is +* exclusively for this transaction. i.e. if the transaction is larger +* than the iclog, it will be the only thing in that iclog. +* Fundamentally, this means we must pass the entire log vector to +* xlog_write to guarantee this. +*/ + iclog_space = log->l_iclog_size - log->l_iclog_hsize; + num_headers = (unit_bytes + iclog_space -1 ) / iclog_space; + +/* for split-recs - ophdrs added when data split over LRs */ + unit_bytes +=sizeof (xlog_op_header_t) * num_headers; + +/* add extra header reservations if we overrun */ +while (!num_headers || + ((unit_bytes + iclog_space -1 ) / iclog_space) > num_headers) { + unit_bytes +=sizeof (xlog_op_header_t); + num_headers++; + } unit_bytes += log->l_iclog_hsize * num_headers;/* for commit-rec LR header - note: padding will subsume the ophdr */ unit_bytes += log->l_iclog_hsize; -/* for split-recs - ophdrs added when data split over LRs */ - unit_bytes +=sizeof (xlog_op_header_t) * num_headers; -/* for roundoff padding for transaction data and one for commit record */ if (xfs_sb_version_haslogv2 (&log->l_mp->m_sb) && log->l_mp->m_sb.sb_logsunit >1 ) {
@@ -3238,7 +3267,7 @@ xlog_ticket_alloc(xlog_t *log, tic->t_trans_type =0 ;if (xflags &XFS_LOG_PERM_RESERV ) tic->t_flags |=XLOG_TIC_PERM_RESERV ; -sv_init (&(tic->t_wait),SV_DEFAULT ,"logtick" ); +sv_init (&tic->t_wait,SV_DEFAULT ,"logtick" );xlog_tic_reset_res (tic);
Signature · 6 lines
1.6.5 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 9/9] xfs: log ticket reservation underestimates the number of iclogs
Quoted 18 lines · Dave Chinner
fs/xfs/xfs_log.c +18
-0+* for LR headers - the space for data in an iclog is the size minus +* the space used for the headers. If we use the iclog size, then we +* undercalculate the number of headers required. +* +* Furthermore - the addition of op headers for split-recs might +* increase the space required enough to require more log and op +* headers, so take that into account too. +* +* IMPORTANT: This reservation makes the assumption that if this +* transaction is the first in an iclog and hence has the LR headers +* accounted to it, then the remaining space in the iclog is +* exclusively for this transaction. i.e. if the transaction is larger +* than the iclog, it will be the only thing in that iclog. +* Fundamentally, this means we must pass the entire log vector to +* xlog_write to guarantee this. +*/ + iclog_space = log->l_iclog_size - log->l_iclog_hsize; + num_headers = (unit_bytes + iclog_space -1 ) / iclog_space;
fs/xfs/xfs_log.c xlog_ticket_alloc(xlog_t *log, · line 3243
Quoted 10 lines
fs/xfs/xfs_log.c +10
-0+ +/* for split-recs - ophdrs added when data split over LRs */ + unit_bytes +=sizeof (xlog_op_header_t) * num_headers; + +/* add extra header reservations if we overrun */ +while (!num_headers || + ((unit_bytes + iclog_space -1 ) / iclog_space) > num_headers) { + unit_bytes +=sizeof (xlog_op_header_t); + num_headers++; + }
Looks good, but why do you check for a zero num_headers here? The only way this could happen after the roundup above is if unit_bytes is zero, which can't ever happen - one caller has it hardcoded to 1, and the the other has a conditional for it beeing bigger than 0 around the call.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 9/9] xfs: log ticket reservation underestimates the number of iclogs
Quoted 9 lines
Quoted 9 lines
fs/xfs/xfs_log.c +9
-0+ iclog_space = log->l_iclog_size - log->l_iclog_hsize; + num_headers = (unit_bytes + iclog_space -1 ) / iclog_space; + +/* for split-recs - ophdrs added when data split over LRs */ + unit_bytes +=sizeof (xlog_op_header_t) * num_headers; + +/* add extra header reservations if we overrun */ +while (!num_headers || + ((unit_bytes + iclog_space -1 ) / iclog_space) > num_headers) {
Quoted 8 lines
fs/xfs/xfs_log.c xlog_ticket_alloc(xlog_t *log, · line 3243
Quoted 3 lines
fs/xfs/xfs_log.c +3
-0+ unit_bytes +=sizeof (xlog_op_header_t); + num_headers++; + }Looks good, but why do you check for a zero
num_headershere? The only way this could happen after the roundup above is ifunit_bytesis zero, which can't ever happen - one caller has it hardcoded to 1, and the the other has a conditional for it beeing bigger than 0 around the call.
BTW: use of the howmany macro might make some of the above easier to read.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 9/9] xfs: log ticket reservation underestimates the number of iclogs
Quoted 9 lines · Christoph Hellwig
Quoted 9 lines
Quoted 9 lines
fs/xfs/xfs_log.c +9
-0+ iclog_space = log->l_iclog_size - log->l_iclog_hsize; + num_headers = (unit_bytes + iclog_space -1 ) / iclog_space; + +/* for split-recs - ophdrs added when data split over LRs */ + unit_bytes +=sizeof (xlog_op_header_t) * num_headers; + +/* add extra header reservations if we overrun */ +while (!num_headers || + ((unit_bytes + iclog_space -1 ) / iclog_space) > num_headers) {
Quoted 8 lines
Quoted 8 lines
fs/xfs/xfs_log.c xlog_ticket_alloc(xlog_t *log, · line 3243
Quoted 3 lines
fs/xfs/xfs_log.c +3
-0+ unit_bytes +=sizeof (xlog_op_header_t); + num_headers++; + }Looks good, but why do you check for a zero
num_headershere? The only way this could happen after the roundup above is ifunit_bytesis zero, which can't ever happen - one caller has it hardcoded to 1, and the the other has a conditional for it beeing bigger than 0 around the call.
Mainly because I want to be able to pass a length of zero in for the checkpoint transaction and have it do the right thing. That is, reserve space for the first header which, in this case, will definitely be used. Indeed, once I made this change, i forgot to change the CIL ticket allocation back to zero - it is still:
tic = xlog_ticket_alloc(log, 1 /* to get a LR header */, 1, XFS_TRANSACTION, 0);
Other than that I can't find any caller that uses unit_bytes = 1. Maybe I missed something - can you point it out to me?
FWIW, there's more missing from the transaction reservations themselves - none of them take into account the space used by the log item format descriptor that describe the changed regions of an object written to the log. They take into account the regions themselves, but not the descriptor. Some of the hard coded transaction reservations do - see all the single sector transactions that reserve "<something> + 128" e.g:
error = xfs_trans_reserve(tp, 0, mp->m_sb.sb_sectsize + 128, 0, 0, XFS_DEFAULT_LOG_COUNT);
That 128 bytes is for the log item format descriptor, and in some cases I'm not sure that 128 bytes is enough. Anyway, that's a problem for a rainy day...
Quoted 2 lines
BTW: use of the howmany macro might make some of the above easier to read.
Good point. I'll look into using that.
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs