xfs: use xfs_get_buf_noaddr for iclogs
PATCH 1/2 xfs: use xfs_get_buf_noaddr for iclogs
Currently xlog_alloc allocates memory for the iclogs first, then allocates a buffer using xfs_buf_get_empty and finally assigns the memory to the buffer. We don't really want to do this, but rather allocate a buffer with memory attached to it using xfs_buf_get_noaddr. There's a subtile change because xfs_buf_get_empty returns the buffer locked, but xfs_buf_get_noaddr returns it unlocked. From my auditing and testing nothing in the log I/O code cares about this distincition, but I'd be happy if someone could try to prove this independently.
fs/xfs/xfs_log.c +8 -10
@@ -1199,11 +1199,16 @@ *iclogp = (xlog_in_core_t *)kmem_zalloc (sizeof (xlog_in_core_t),KM_SLEEP ); iclog = *iclogp; - iclog->hic_data = (xlog_in_core_2_t *) -kmem_zalloc (iclogsize,KM_SLEEP |KM_LARGE ); - iclog->ic_prev = prev_iclog; prev_iclog = iclog; + + bp =xfs_buf_get_noaddr (log->l_iclog_size, mp->m_logdev_targp); +XFS_BUF_SET_IODONE_FUNC (bp, xlog_iodone); +XFS_BUF_SET_BDSTRAT_FUNC (bp, xlog_bdstrat_cb); +XFS_BUF_SET_FSPRIVATE2 (bp, (unsigned long )1 ); + iclog->ic_bp = bp; + iclog->hic_data = bp->b_addr; + log->l_iclog_bak[i] = (xfs_caddr_t)&(iclog->ic_header); head = &iclog->ic_header;
@@ -1216,11 +1221,6 @@INT_SET (head->h_fmt,ARCH_CONVERT ,XLOG_FMT );memcpy (&head->h_fs_uuid, &mp->m_sb.sb_uuid,sizeof (uuid_t)); - bp =xfs_buf_get_empty (log->l_iclog_size, mp->m_logdev_targp); -XFS_BUF_SET_IODONE_FUNC (bp, xlog_iodone); -XFS_BUF_SET_BDSTRAT_FUNC (bp, xlog_bdstrat_cb); -XFS_BUF_SET_FSPRIVATE2 (bp, (unsigned long )1 ); - iclog->ic_bp = bp; iclog->ic_size =XFS_BUF_SIZE (bp) - log->l_iclog_hsize; iclog->ic_state =XLOG_STATE_ACTIVE ;
@@ -1229,7 +1229,6 @@ iclog->ic_datap = (char *)iclog->hic_data + log->l_iclog_hsize;ASSERT (XFS_BUF_ISBUSY (iclog->ic_bp)); -ASSERT (XFS_BUF_VALUSEMA (iclog->ic_bp) <=0 );sv_init (&iclog->ic_forcesema,SV_DEFAULT ,"iclog-force" );sv_init (&iclog->ic_writesema,SV_DEFAULT ,"iclog-write" );
@@ -1528,7 +1527,6 @@ }#endif next_iclog = iclog->ic_next; -kmem_free (iclog->hic_data, log->l_iclog_size);kmem_free (iclog,sizeof (xlog_in_core_t)); iclog = next_iclog; }
Does not look to me either. Looks logical as well because these buffers are used only in log syncing and only one thread can be ever flushing one ICLOG and, hence, no need for protection.
Even split buffer (log->l_xbuf) is used by only ICLOG at a time,
should not matter. I don't see protection for this even today as no locking is done in split sync path.
-shailendra
Quoted 67 lines · Christoph Hellwig
Currently
xlog_allocallocates memory for the iclogs first, then allocates a buffer usingxfs_buf_get_emptyand finally assigns the memory to the buffer. We don't really want to do this, but rather allocate a buffer with memory attached to it usingxfs_buf_get_noaddr. There's a subtile change becausexfs_buf_get_emptyreturns the buffer locked, butxfs_buf_get_noaddrreturns it unlocked. From my auditing and testing nothing in the log I/O code cares about this distincition, but I'd be happy if someone could try to prove this independently.Signed-off-by Christoph Hellwig <[email protected]>Index:
linux-2.6/fs/xfs/xfs_log.c
===================================================================fs/xfs/xfs_log.c +8
-10@@ -1199,11 +1199,16 @@ *iclogp = (xlog_in_core_t *)kmem_zalloc (sizeof (xlog_in_core_t),KM_SLEEP ); iclog = *iclogp; - iclog->hic_data = (xlog_in_core_2_t *) -kmem_zalloc (iclogsize,KM_SLEEP |KM_LARGE ); - iclog->ic_prev = prev_iclog; prev_iclog = iclog; + + bp =xfs_buf_get_noaddr (log->l_iclog_size, mp->m_logdev_targp); +XFS_BUF_SET_IODONE_FUNC (bp, xlog_iodone); +XFS_BUF_SET_BDSTRAT_FUNC (bp, xlog_bdstrat_cb); +XFS_BUF_SET_FSPRIVATE2 (bp, (unsigned long )1 ); + iclog->ic_bp = bp; + iclog->hic_data = bp->b_addr; + log->l_iclog_bak[i] = (xfs_caddr_t)&(iclog->ic_header); head = &iclog->ic_header;@@ -1216,11 +1221,6 @@INT_SET (head->h_fmt,ARCH_CONVERT ,XLOG_FMT );memcpy (&head->h_fs_uuid, &mp->m_sb.sb_uuid,sizeof (uuid_t)); - bp =xfs_buf_get_empty (log->l_iclog_size, mp->m_logdev_targp); -XFS_BUF_SET_IODONE_FUNC (bp, xlog_iodone); -XFS_BUF_SET_BDSTRAT_FUNC (bp, xlog_bdstrat_cb); -XFS_BUF_SET_FSPRIVATE2 (bp, (unsigned long )1 ); - iclog->ic_bp = bp; iclog->ic_size =XFS_BUF_SIZE (bp) - log->l_iclog_hsize; iclog->ic_state =XLOG_STATE_ACTIVE ;@@ -1229,7 +1229,6 @@ iclog->ic_datap = (char *)iclog->hic_data + log->l_iclog_hsize;ASSERT (XFS_BUF_ISBUSY (iclog->ic_bp)); -ASSERT (XFS_BUF_VALUSEMA (iclog->ic_bp) <=0 );sv_init (&iclog->ic_forcesema,SV_DEFAULT ,"iclog-force" );sv_init (&iclog->ic_writesema,SV_DEFAULT ,"iclog-write" );@@ -1528,7 +1527,6 @@ }#endif next_iclog = iclog->ic_next; -kmem_free (iclog->hic_data, log->l_iclog_size);kmem_free (iclog,sizeof (xlog_in_core_t)); iclog = next_iclog; }
Quoted 5 lines · Christoph Hellwig
xfs_buf_get_noaddr. There's a subtile change becausexfs_buf_get_emptyreturns the buffer locked, butxfs_buf_get_noaddrreturns it unlocked. From my auditing and testing nothing in the log I/O code cares about this distincition, but I'd be happy if someone could try to prove this independently.
Looks safe to me - we initialise all the fields in the xfs_buf_t when we allocate out of the slab, so it doesn't really matter what state the buffer is in when we free it.
OTOH, all other buffers are supposed to be locked when under I/O. This change makes a special case for the log buffers, and I'd prefer not to have to remember that this behaviour changed fo log buffers at some point in time.
I suggest that adding:
fs/xfs/xfs_log.c line 1208
Quoted 10 lines
fs/xfs/xfs_log.c +5
-3- iclog->hic_data = (xlog_in_core_2_t *) -kmem_zalloc (iclogsize,KM_SLEEP |KM_LARGE ); - iclog->ic_prev = prev_iclog; prev_iclog = iclog; + + bp =xfs_buf_get_noaddr (log->l_iclog_size, mp->m_logdev_targp); +XFS_BUF_SET_IODONE_FUNC (bp, xlog_iodone); +XFS_BUF_SET_BDSTRAT_FUNC (bp, xlog_bdstrat_cb); +XFS_BUF_SET_FSPRIVATE2 (bp, (unsigned long )1 );
+ XFS_BUF_PSEMA(bp, PRIBIO);
fs/xfs/xfs_log.c line 1214
Quoted 6 lines
fs/xfs/xfs_log.c +3
-0+ iclog->ic_bp = bp; + iclog->hic_data = bp->b_addr; + log->l_iclog_bak[i] = (xfs_caddr_t)&(iclog->ic_header); head = &iclog->ic_header;
To lock the buffer should be added here. That way we don't change any semantics of the code at all.
Quoted 13 lines
fs/xfs/xfs_log.c +0
-5@@ -1216,11 +1221,6 @@INT_SET (head->h_fmt,ARCH_CONVERT ,XLOG_FMT );memcpy (&head->h_fs_uuid, &mp->m_sb.sb_uuid,sizeof (uuid_t)); - bp =xfs_buf_get_empty (log->l_iclog_size, mp->m_logdev_targp); -XFS_BUF_SET_IODONE_FUNC (bp, xlog_iodone); -XFS_BUF_SET_BDSTRAT_FUNC (bp, xlog_bdstrat_cb); -XFS_BUF_SET_FSPRIVATE2 (bp, (unsigned long )1 ); - iclog->ic_bp = bp; iclog->ic_size =XFS_BUF_SIZE (bp) - log->l_iclog_hsize; iclog->ic_state =XLOG_STATE_ACTIVE ;
fs/xfs/xfs_log.c line 1232
Quoted 4 lines
fs/xfs/xfs_log.c +0
-1@@ -1229,7 +1229,6 @@ iclog->ic_datap = (char *)iclog->hic_data + log->l_iclog_hsize;ASSERT (XFS_BUF_ISBUSY (iclog->ic_bp)); -ASSERT (XFS_BUF_VALUSEMA (iclog->ic_bp) <=0 );
And this assert can then stay...
Cheers,
Dave.
Signature · 3 lines
Dave Chinner Principal Engineer SGI Australian Software Group
Quoted 6 lines · David Chinner
OTOH, all other buffers are supposed to be locked when under I/O. This change makes a special case for the log buffers, and I'd prefer not to have to remember that this behaviour changed fo log buffers at some point in time.
I suggest that adding:
...
Quoted 1 line
(quoted diff) +1
-0+ XFS_BUF_PSEMA(bp, PRIBIO);
...
Quoted 2 lines
To lock the buffer should be added here. That way we don't change any semantics of the code at all.
Here's a patch with your suggestion implemented. Seems to work fine under heavy NFS load for me. Note that the log recovery has some inconsistancies already about doing I/O both on locked and unlocked buffers. Long-term it might be a good idea to change xfs_get_buf_noaddr to return a locked buffer like xfs_get_buf(_flags) does already.
fs/xfs/xfs_log.c +10 -9
@@ -1199,11 +1199,18 @@ xlog_alloc_log(xfs_mount_t *mp, *iclogp = (xlog_in_core_t *)kmem_zalloc (sizeof (xlog_in_core_t),KM_SLEEP ); iclog = *iclogp; - iclog->hic_data = (xlog_in_core_2_t *) -kmem_zalloc (iclogsize,KM_SLEEP |KM_LARGE ); - iclog->ic_prev = prev_iclog; prev_iclog = iclog; + + bp =xfs_buf_get_noaddr (log->l_iclog_size, mp->m_logdev_targp); +if (!XFS_BUF_CPSEMA (bp)) +ASSERT (0 ); +XFS_BUF_SET_IODONE_FUNC (bp, xlog_iodone); +XFS_BUF_SET_BDSTRAT_FUNC (bp, xlog_bdstrat_cb); +XFS_BUF_SET_FSPRIVATE2 (bp, (unsigned long )1 ); + iclog->ic_bp = bp; + iclog->hic_data = bp->b_addr; + log->l_iclog_bak[i] = (xfs_caddr_t)&(iclog->ic_header); head = &iclog->ic_header;
@@ -1216,11 +1223,6 @@ xlog_alloc_log(xfs_mount_t *mp,INT_SET (head->h_fmt,ARCH_CONVERT ,XLOG_FMT );memcpy (&head->h_fs_uuid, &mp->m_sb.sb_uuid,sizeof (uuid_t)); - bp =xfs_buf_get_empty (log->l_iclog_size, mp->m_logdev_targp); -XFS_BUF_SET_IODONE_FUNC (bp, xlog_iodone); -XFS_BUF_SET_BDSTRAT_FUNC (bp, xlog_bdstrat_cb); -XFS_BUF_SET_FSPRIVATE2 (bp, (unsigned long )1 ); - iclog->ic_bp = bp; iclog->ic_size =XFS_BUF_SIZE (bp) - log->l_iclog_hsize; iclog->ic_state =XLOG_STATE_ACTIVE ;
@@ -1528,7 +1530,6 @@ xlog_dealloc_log(xlog_t *log) }#endif next_iclog = iclog->ic_next; -kmem_free (iclog->hic_data, log->l_iclog_size);kmem_free (iclog,sizeof (xlog_in_core_t)); iclog = next_iclog; }