xfs: make log devices with write back caches work
PATCH 1/1 xfs: make log devices with write back caches work
There's no reason not to support cache flushing on external log devices. The only thing this really requires is flushing the data device first both in fsync and log commits. A side effect is that we also have to remove the barrier write test during mount, which has been superflous since the new FLUSH+FUA code anyway. Also use the chance to flush the RT subvolume write cache before the fsync commit, which is required for correct semantics.
fs/xfs/linux-2.6/xfs_file.c +27 -19
@@ -131,19 +131,32 @@ xfs_file_fsync( {struct inode *inode = file->f_mapping->host;struct xfs_inode *ip =XFS_I (inode); +struct xfs_mount *mp = ip->i_mount;struct xfs_trans *tp;int error =0 ;int log_flushed =0 ;trace_xfs_file_fsync (ip); -if (XFS_FORCED_SHUTDOWN (ip->i_mount)) +if (XFS_FORCED_SHUTDOWN (mp))return -XFS_ERROR (EIO );xfs_iflags_clear (ip,XFS_ITRUNCATED );xfs_ioend_wait (ip); +if (mp->m_flags &XFS_MOUNT_BARRIER ) { +/* +* If we have an RT and/or log subvolume we need to make sure +* to flush the write cache on the device that the data +* resides before commit the transaction. +*/ +if (XFS_IS_REALTIME_INODE (ip)) +xfs_blkdev_issue_flush (mp->m_rtdev_targp); +else if (mp->m_logdev_targp != mp->m_ddev_targp) +xfs_blkdev_issue_flush (mp->m_ddev_targp); + } +/* * We always need to make sure that the required inode state is safe on * disk. The inode might be clean but we still might need to force the
@@ -175,9 +188,9 @@ xfs_file_fsync(* updates. The sync transaction will also force the log. */ xfs_iunlock (ip,XFS_ILOCK_SHARED ); - tp =xfs_trans_alloc (ip->i_mount,XFS_TRANS_FSYNC_TS ); + tp =xfs_trans_alloc (mp,XFS_TRANS_FSYNC_TS ); error =xfs_trans_reserve (tp,0 , -XFS_FSYNC_TS_LOG_RES (ip->i_mount),0 ,0 ,0 ); +XFS_FSYNC_TS_LOG_RES (mp),0 ,0 ,0 );if (error) {xfs_trans_cancel (tp,0 );return -error;
@@ -209,28 +222,23 @@ xfs_file_fsync(* force the log. */ if (xfs_ipincount (ip)) { - error =_xfs_log_force_lsn (ip->i_mount, + error =_xfs_log_force_lsn (mp, ip->i_itemp->ili_last_lsn,XFS_LOG_SYNC , &log_flushed); }xfs_iunlock (ip,XFS_ILOCK_SHARED ); } -if (ip->i_mount->m_flags &XFS_MOUNT_BARRIER ) { -/* -* If the log write didn't issue an ordered tag we need -* to flush the disk cache for the data device now. -*/ -if (!log_flushed) -xfs_blkdev_issue_flush (ip->i_mount->m_ddev_targp); - -/* -* If this inode is on the RT dev we need to flush that -* cache as well. -*/ -if (XFS_IS_REALTIME_INODE (ip)) -xfs_blkdev_issue_flush (ip->i_mount->m_rtdev_targp); - } +/* +* Flush the write cache on the data volume, if the data for this +* inode actually resides on it, and the transaction commit did +* not flush it yet. +*/ +if ((mp->m_flags &XFS_MOUNT_BARRIER ) && + mp->m_logdev_targp == mp->m_ddev_targp && + !XFS_IS_REALTIME_INODE (ip) && + !log_flushed) +xfs_blkdev_issue_flush (mp->m_ddev_targp);return -error; }
fs/xfs/linux-2.6/xfs_super.c +0 -75
99 lines not on this page · raw message
fs/xfs/xfs_log.c +8 -1
@@ -1372,8 +1372,15 @@ xlog_sync(xlog_t *log,XFS_BUF_ASYNC (bp); bp->b_flags |=XBF_LOG_BUFFER ; -if (log->l_mp->m_flags &XFS_MOUNT_BARRIER ) +if (log->l_mp->m_flags &XFS_MOUNT_BARRIER ) { +/* +* If we have an external log device, flush the data device +* before flushing the log. +*/ +if (log->l_mp->m_logdev_targp != log->l_mp->m_ddev_targp) +xfs_blkdev_issue_flush (log->l_mp->m_ddev_targp);XFS_BUF_ORDERED (bp); + }ASSERT (XFS_BUF_ADDR (bp) <= log->l_logBBsize-1 );ASSERT (XFS_BUF_ADDR (bp) +BTOBB (count) <= log->l_logBBsize);
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 2 lines · Christoph Hellwig
There's no reason not to support cache flushing on external log devices. The only thing this really requires is flushing the data device first
I know this is just the description but it wasn't obvious to me initially the reason for this ordering. Now I know: it's to ensure any data written to new space on a file extending write is on disk before a size change gets committed. Maybe you could mention this (in addition to mentioning it in a comment, as I suggest below.
(And I don't know if the case mentioned above is the only one where this ordering is important. It's conceivable there could be a case where the reverse ordering (log first) was needed, but I have not thought that through at all and I presume not.)
Quoted 5 lines
both in fsync and log commits. A side effect is that we also have to remove the barrier write test during mount, which has been superflous since the new FLUSH+FUA code anyway. Also use the chance to flush the RT subvolume write cache before the fsync commit, which is required for correct semantics.
A few comments below.
fs/xfs/linux-2.6/xfs_file.c xfs_file_fsync( · line 133
Quoted 10 lines
Signed-off-by Christoph Hellwig <[email protected]>Index:
xfs/fs/xfs/linux-2.6/xfs_file.c
===================================================================fs/xfs/linux-2.6/xfs_file.c +0
-0@@ -131,19 +131,32 @@ xfs_file_fsync( {struct inode *inode = file->f_mapping->host;struct xfs_inode *ip =XFS_I (inode);
. . .
fs/xfs/linux-2.6/xfs_file.c xfs_file_fsync( · line 155
Quoted 10 lines
fs/xfs/linux-2.6/xfs_file.c +8
-0xfs_ioend_wait (ip); +if (mp->m_flags &XFS_MOUNT_BARRIER ) { +/* +* If we have an RT and/or log subvolume we need to make sure +* to flush the write cache on the device that the data +* resides before commit the transaction. +*/ +if (XFS_IS_REALTIME_INODE (ip)) +xfs_blkdev_issue_flush (mp->m_rtdev_targp);
I questioned the "else" below. But as you mentioned on IRC it makes sense because a realtime inode will never share the same device target as the log (or later in this function, the data device). A short mention of that in the comment above might be reassuring. Similarly, I think it would be worthwhile to mention the reason for doing this flush here *before* the log gets forced.
fs/xfs/linux-2.6/xfs_file.c xfs_file_fsync( · line 162
Quoted 7 lines
fs/xfs/linux-2.6/xfs_file.c +4
-0+else if (mp->m_logdev_targp != mp->m_ddev_targp) +xfs_blkdev_issue_flush (mp->m_ddev_targp); + } +/* * We always need to make sure that the required inode state is safe on * disk. The inode might be clean but we still might need to force the
. . .
fs/xfs/linux-2.6/xfs_file.c xfs_file_fsync( · line 227
Quoted 8 lines
fs/xfs/linux-2.6/xfs_file.c +1
-1@@ -209,28 +222,23 @@ xfs_file_fsync(* force the log. */ if (xfs_ipincount (ip)) { - error =_xfs_log_force_lsn (ip->i_mount, + error =_xfs_log_force_lsn (mp, ip->i_itemp->ili_last_lsn,XFS_LOG_SYNC , &log_flushed);
If log_flushed is set on return here, does that also imply that the actual blkdev flush of the log device (which is possibly the data device) has occurred? It looks to me like xlog_sync() marks each log buffer ordered but I don't see the flush (though I didn't look *that* hard...).
Quoted 19 lines
fs/xfs/linux-2.6/xfs_file.c +0
-15}xfs_iunlock (ip,XFS_ILOCK_SHARED ); } -if (ip->i_mount->m_flags &XFS_MOUNT_BARRIER ) { -/* -* If the log write didn't issue an ordered tag we need -* to flush the disk cache for the data device now. -*/ -if (!log_flushed) -xfs_blkdev_issue_flush (ip->i_mount->m_ddev_targp); - -/* -* If this inode is on the RT dev we need to flush that -* cache as well. -*/ -if (XFS_IS_REALTIME_INODE (ip)) -xfs_blkdev_issue_flush (ip->i_mount->m_rtdev_targp); - }
fs/xfs/linux-2.6/xfs_file.c xfs_file_fsync( · line 241
Quoted 10 lines
fs/xfs/linux-2.6/xfs_file.c +10
-0+/* +* Flush the write cache on the data volume, if the data for this +* inode actually resides on it, and the transaction commit did +* not flush it yet. +*/ +if ((mp->m_flags &XFS_MOUNT_BARRIER ) && + mp->m_logdev_targp == mp->m_ddev_targp && + !XFS_IS_REALTIME_INODE (ip) && + !log_flushed) +xfs_blkdev_issue_flush (mp->m_ddev_targp);
Note my previous comment. It kind of arose because my thought when seeing this was "What if it is an internal log but _xfs_log_force_lsn() does *not* issue a blkdev flush to the log/data device?"
Breaking it down, we have either one or two devices to flush, the log and either the realtime or (if external log) the data device:
- realtime device (flush it before log device flush)
- data device (only if not realtime; do first if external log)
- log device (flush this last, realtime or not)
I may be wrong, but it seems like this may be the
logic you need:
if (RT)
flush RT device
else if (external log)
flush data device
force the log
if (log device wasn't flushed when log forced)
flush log device
The log device flush of course implies a data device flush for an internal log.
fs/xfs/linux-2.6/xfs_super.c xfs_blkdev_put( · line 628
Quoted 9 lines
fs/xfs/linux-2.6/xfs_file.c +0
-0return -error; }Index:
xfs/fs/xfs/linux-2.6/xfs_super.c
===================================================================fs/xfs/linux-2.6/xfs_super.c +0
-0@@ -627,68 +627,6 @@ xfs_blkdev_put(blkdev_put (bdev,FMODE_READ |FMODE_WRITE |FMODE_EXCL ); }
. . .
fs/xfs/linux-2.6/xfs_super.c xfs_blkdev_put( · line 666
Quoted 4 lines
fs/xfs/linux-2.6/xfs_super.c +0
-4- -STATIC void -xfs_mountfs_check_barriers (xfs_mount_t *mp) -{
. . .
fs/xfs/linux-2.6/xfs_super.c xfs_blkdev_put( · line 681
Quoted 6 lines
fs/xfs/linux-2.6/xfs_super.c +0
-6-if (xfs_readonly_buftarg (mp->m_ddev_targp)) { -xfs_notice (mp, -"Disabling barriers, underlying device is readonly" ); - mp->m_flags &= ~XFS_MOUNT_BARRIER ; -return ; - }
At first I was going to suggest this might still be a worthwhile optimization. But I think a read-only underlying device will prevent us from ever reaching the points that (now) would bother checking the XFS_MOUNT_BARRIER flag.
fs/xfs/linux-2.6/xfs_super.c xfs_blkdev_put( · line 632
Quoted 13 lines
fs/xfs/linux-2.6/xfs_super.c +0
-10- - error =xfs_barrier_test (mp); -if (error) { -xfs_notice (mp, -"Disabling barriers, trial barrier write failed" ); - mp->m_flags &= ~XFS_MOUNT_BARRIER ; -return ; - } -} -void xfs_blkdev_issue_flush (xfs_buftarg_t *buftarg)
. . .
Quoted 6 lines
Index:
xfs/fs/xfs/xfs_log.c
===================================================================fs/xfs/xfs_log.c +0
-0@@ -1372,8 +1372,15 @@ xlog_sync(xlog_t *log,XFS_BUF_ASYNC (bp);
fs/xfs/xfs_log.c xlog_sync(xlog_t *log, · line 1381
Quoted 10 lines
fs/xfs/xfs_log.c +7
-1bp->b_flags |=XBF_LOG_BUFFER ; -if (log->l_mp->m_flags &XFS_MOUNT_BARRIER ) +if (log->l_mp->m_flags &XFS_MOUNT_BARRIER ) { +/* +* If we have an external log device, flush the data device +* before flushing the log. +*/ +if (log->l_mp->m_logdev_targp != log->l_mp->m_ddev_targp) +xfs_blkdev_issue_flush (log->l_mp->m_ddev_targp);
Shouldn't we flush the realtime device as well here, if present?
Quoted 10 lines
fs/xfs/xfs_log.c +1
-0XFS_BUF_ORDERED (bp); + }ASSERT (XFS_BUF_ADDR (bp) <= log->l_logBBsize-1 );ASSERT (XFS_BUF_ADDR (bp) +BTOBB (count) <= log->l_logBBsize);_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 7 lines · Alex Elder
I questioned the "else" below. But as you mentioned on IRC it makes sense because a realtime inode will never share the same device target as the log (or later in this function, the data device). A short mention of that in the comment above might be reassuring. Similarly, I think it would be worthwhile to mention the reason for doing this flush here *before* the log gets forced.
I'll update the comments.
Quoted 5 lines
If
log_flushedis set on return here, does that also imply that the actual blkdev flush of the log device (which is possibly the data device) has occurred? It looks to me likexlog_sync()marks each log buffer ordered but I don't see the flush (though I didn't look *that* hard...).
Yes. The XBF_ORDERED flag is a bit misnamed, but it causes the to set REQ_FLUSH_FUA and thus a full cache flush before the actual write.
I have some patches in my queue for the next cycle to clean that naming up.
Quoted 6 lines
Breaking it down, we have either one or two devices to flush, the log and either the realtime or (if external log) the data device:
- realtime device (flush it before log device flush)
- data device (only if not realtime; do first if external log)
- log device (flush this last, realtime or not)
If we have both an RT and a log device we might even have to flush all three. But the flush of the main device is trasparently handled in xlog_sync(). At this point we might double-flush the data device, but cleaning that up is more involved and I'll leave it for the next cycle.
Quoted 12 lines
I may be wrong, but it seems like this may be the logic you need:
if (RT)
flush RT device
else if (external log)
flush data device
force the log
if (log device wasn't flushed when log forced)
flush log device
Quoted 2 lines
The log device flush of course implies a data device flush for an internal log.
No, that's incorrect. If we have dirty data in the log that needs to be flushed (that is anything dirty for this inode is in the log), the log force does it for us.
The flush of the data device after the log force is for the case where the log did not get flushed. That case only happens if we did overwrites of and already allocated file, and we're using fdatasync and thus don't flush out the timestamps.
Quoted 10 lines
fs/xfs/xfs_log.c xlog_sync(xlog_t *log, · line 1381
Quoted 8 lines
fs/xfs/xfs_log.c +7
-1-if (log->l_mp->m_flags &XFS_MOUNT_BARRIER ) +if (log->l_mp->m_flags &XFS_MOUNT_BARRIER ) { +/* +* If we have an external log device, flush the data device +* before flushing the log. +*/ +if (log->l_mp->m_logdev_targp != log->l_mp->m_ddev_targp) +xfs_blkdev_issue_flush (log->l_mp->m_ddev_targp);Shouldn't we flush the realtime device as well here, if present?
No. The reason we flush the data device here is to make sure metadata written back from the AIL actually made it to disk before moving the log tail. None of that metadata is on the RT device.
I'll add a comment explaning what we do here.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs