xfs
[Top] [All Lists]

Re: [PATCH 1/2] xfs: use xfs_get_buf_noaddr for iclogs

To: Christoph Hellwig <hch@xxxxxx>
Subject: Re: [PATCH 1/2] xfs: use xfs_get_buf_noaddr for iclogs
From: Shailendra Tripathi <stripathi@xxxxxxxxx>
Date: Wed, 07 Mar 2007 17:29:40 +0530
Cc: xfs@xxxxxxxxxxx, ecashin@xxxxxxxxxx, akpm@xxxxxxxx, linux-kernel@xxxxxxxxxxxxxxx
In-reply-to: <20070307101314.GB30587@xxxxxx>
References: <20070307101314.GB30587@xxxxxx>
Sender: xfs-bounce@xxxxxxxxxxx
User-agent: Mozilla Thunderbird 0.9 (X11/20041127)
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

Christoph Hellwig wrote:
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.


Signed-off-by: Christoph Hellwig <hch@xxxxxx>

Index: linux-2.6/fs/xfs/xfs_log.c
===================================================================
--- linux-2.6.orig/fs/xfs/xfs_log.c     2007-03-06 17:26:40.000000000 +0100
+++ linux-2.6/fs/xfs/xfs_log.c  2007-03-06 17:28:03.000000000 +0100
@@ -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;
        }




<Prev in Thread] Current Thread [Next in Thread>