Skip to contentSkip to conversation

XFS_BUF_ORDERED instead of XFS_BUF_ISORDERED?

DISCUSSION Nathaniel W. Turner 3 participants 3 messages Aug 25, 2008 – Aug 26, 2008 XFS filesystem

lore
Share & tools
lore

Nathaniel W. Turner

author
#0 lore raw

Hi folks,

While doing a little light reading, I noticed the following in fs/xfs/xfs_log.c:1011 (in the 2.6.26.3 Linux kernel tree). Am I missing something, or should XFS_BUF_ORDERED be replaced with XFS_BUF_ISORDERED in this check?

nate

/*
* If the ordered flag has been removed by a lower
* layer, it means the underlyin device no longer supports
* barrier I/O. Warn loudly and turn off barriers. */
if ((l->l_mp->m_flags & XFS_MOUNT_BARRIER) &&

!XFS_BUF_ORDERED(bp)) {
               l->l_mp->m_flags &= ~XFS_MOUNT_BARRIER;
               xfs_fs_cmn_err(CE_WARN, l->l_mp,
                               "xlog_iodone: Barriers are no longer 
supported"
                               " by device. Disabling barriers\n");
               xfs_buftrace("XLOG_IODONE BARRIERS OFF", bp);
       }
#1 lore raw
Quoted 16 lines · Nathaniel W. Turner

Hi folks,

While doing a little light reading, I noticed the following in fs/xfs/xfs_log.c:1011 (in the 2.6.26.3 Linux kernel tree). Am I missing something, or should XFS_BUF_ORDERED be replaced with XFS_BUF_ISORDERED in this check?

nate

/*
* If the ordered flag has been removed by a lower
* layer, it means the underlyin device no longer supports
* barrier I/O. Warn loudly and turn off barriers. */
if ((l->l_mp->m_flags & XFS_MOUNT_BARRIER) &&

Quoted 10 lines
!XFS_BUF_ORDERED(bp)) {
                l->l_mp->m_flags &= ~XFS_MOUNT_BARRIER;
                xfs_fs_cmn_err(CE_WARN, l->l_mp,
                                "xlog_iodone: Barriers are no longer 
supported"
                                " by device. Disabling barriers\n");
                xfs_buftrace("XLOG_IODONE BARRIERS OFF", bp);
        }

wow, I think you're right.

Fortunately I dont' think this does much harm in most cases - the buffer is usually ordered here anyway, and we're re-setting it instead of testing it, as you noticed. But it's making this whole dynamic-lack-of-barrier detection pointless/broken.

modest proposal: change the macro (and friends) to XFS_BUF_SETORDERED or somesuch. :)

-Eric

#2 lore raw
In reply to Nathaniel W. Turner
Quoted 6 lines · Nathaniel W. Turner

Hi folks,

While doing a little light reading, I noticed the following in fs/xfs/xfs_log.c:1011 (in the 2.6.26.3 Linux kernel tree). Am I missing something, or should XFS_BUF_ORDERED be replaced with XFS_BUF_ISORDERED in this check?

Yes, it is wrong. Well spotted. I guess this shows that I was right when I complained about needing this untestable barrier handling code in every filesystem....

Patch below.

Cheers,

Dave.

Signature · 14 lines
Dave Chinner
[email protected]


XFS: Fix barrier status change detection

The current code in xlog_iodone() uses the wrong macro
to check if the barrier has been cleared due to an EOPNOTSUPP
error form the lower layer.

Signed-off-by: Dave Chinner <[email protected]>
---
 fs/xfs/xfs_log.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
fs/xfs/xfs_log.c +1 -1
index ccba14e..ff2ac20 100644
@@ -1009,7 +1009,7 @@ xlog_iodone(xfs_buf_t *bp)
 	 * layer, it means the underlyin device no longer supports
 	 * barrier I/O. Warn loudly and turn off barriers.
 	 */
-	if ((l->l_mp->m_flags & XFS_MOUNT_BARRIER) && !XFS_BUF_ORDERED(bp)) {
+	if ((l->l_mp->m_flags & XFS_MOUNT_BARRIER) && !XFS_BUF_ISORDERED(bp)) {
 		l->l_mp->m_flags &= ~XFS_MOUNT_BARRIER;
 		xfs_fs_cmn_err(CE_WARN, l->l_mp,
 				"xlog_iodone: Barriers are no longer supported"