* [PATCH] remove dead SYNC_BDFLUSH case in xfs_sync_inodes
@ 2007-09-09 15:42 Christoph Hellwig
2007-09-09 19:43 ` Bhagi rathi
0 siblings, 1 reply; 4+ messages in thread
From: Christoph Hellwig @ 2007-09-09 15:42 UTC (permalink / raw)
To: xfs
A large part of xfs_sync_inodes is conditional on the SYNC_BDFLUSH which
is never passed to it. This patch removes it and adds an assert that
triggers in case some new code tries to pass SYNC_BDFLUSH to it.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Index: linux-2.6-xfs/fs/xfs/xfs_vfsops.c
===================================================================
--- linux-2.6-xfs.orig/fs/xfs/xfs_vfsops.c 2007-09-08 17:43:39.000000000 +0200
+++ linux-2.6-xfs/fs/xfs/xfs_vfsops.c 2007-09-08 19:06:31.000000000 +0200
@@ -981,8 +981,6 @@ xfs_sync_inodes(
int *bypassed)
{
xfs_inode_t *ip = NULL;
- xfs_inode_t *ip_next;
- xfs_buf_t *bp;
bhv_vnode_t *vp = NULL;
int error;
int last_error;
@@ -992,7 +990,6 @@ xfs_sync_inodes(
boolean_t mount_locked;
boolean_t vnode_refed;
int preempt;
- xfs_dinode_t *dip;
xfs_iptr_t *ipointer;
#ifdef DEBUG
boolean_t ipointer_in = B_FALSE;
@@ -1045,6 +1042,8 @@ xfs_sync_inodes(
#define XFS_PREEMPT_MASK 0x7f
+ ASSERT(!(flags & SYNC_BDFLUSH));
+
if (bypassed)
*bypassed = 0;
if (mp->m_flags & XFS_MOUNT_RDONLY)
@@ -1057,7 +1056,7 @@ xfs_sync_inodes(
ipointer = (xfs_iptr_t *)kmem_zalloc(sizeof(xfs_iptr_t), KM_SLEEP);
fflag = XFS_B_ASYNC; /* default is don't wait */
- if (flags & (SYNC_BDFLUSH | SYNC_DELWRI))
+ if (flags & SYNC_DELWRI)
fflag = XFS_B_DELWRI;
if (flags & SYNC_WAIT)
fflag = 0; /* synchronous overrides all */
@@ -1147,24 +1146,6 @@ xfs_sync_inodes(
}
/*
- * If this is just vfs_sync() or pflushd() calling
- * then we can skip inodes for which it looks like
- * there is nothing to do. Since we don't have the
- * inode locked this is racy, but these are periodic
- * calls so it doesn't matter. For the others we want
- * to know for sure, so we at least try to lock them.
- */
- if (flags & SYNC_BDFLUSH) {
- if (((ip->i_itemp == NULL) ||
- !(ip->i_itemp->ili_format.ilf_fields &
- XFS_ILOG_ALL)) &&
- (ip->i_update_core == 0)) {
- ip = ip->i_mnext;
- continue;
- }
- }
-
- /*
* Try to lock without sleeping. We're out of order with
* the inode list lock here, so if we fail we need to drop
* the mount lock and try again. If we're called from
@@ -1181,7 +1162,7 @@ xfs_sync_inodes(
* it.
*/
if (xfs_ilock_nowait(ip, lock_flags) == 0) {
- if ((flags & SYNC_BDFLUSH) || (vp == NULL)) {
+ if (vp == NULL) {
ip = ip->i_mnext;
continue;
}
@@ -1242,160 +1223,27 @@ xfs_sync_inodes(
xfs_ilock(ip, XFS_ILOCK_SHARED);
}
- if (flags & SYNC_BDFLUSH) {
- if ((flags & SYNC_ATTR) &&
- ((ip->i_update_core) ||
- ((ip->i_itemp != NULL) &&
- (ip->i_itemp->ili_format.ilf_fields != 0)))) {
-
- /* Insert marker and drop lock if not already
- * done.
- */
- if (mount_locked) {
- IPOINTER_INSERT(ip, mp);
- }
-
- /*
- * We don't want the periodic flushing of the
- * inodes by vfs_sync() to interfere with
- * I/O to the file, especially read I/O
- * where it is only the access time stamp
- * that is being flushed out. To prevent
- * long periods where we have both inode
- * locks held shared here while reading the
- * inode's buffer in from disk, we drop the
- * inode lock while reading in the inode
- * buffer. We have to release the buffer
- * and reacquire the inode lock so that they
- * are acquired in the proper order (inode
- * locks first). The buffer will go at the
- * end of the lru chain, though, so we can
- * expect it to still be there when we go
- * for it again in xfs_iflush().
- */
- if ((xfs_ipincount(ip) == 0) &&
- xfs_iflock_nowait(ip)) {
-
- xfs_ifunlock(ip);
- xfs_iunlock(ip, XFS_ILOCK_SHARED);
-
- error = xfs_itobp(mp, NULL, ip,
- &dip, &bp, 0, 0);
- if (!error) {
- xfs_buf_relse(bp);
- } else {
- /* Bailing out, remove the
- * marker and free it.
- */
- XFS_MOUNT_ILOCK(mp);
- IPOINTER_REMOVE(ip, mp);
- XFS_MOUNT_IUNLOCK(mp);
-
- ASSERT(!(lock_flags &
- XFS_IOLOCK_SHARED));
-
- kmem_free(ipointer,
- sizeof(xfs_iptr_t));
- return (0);
- }
-
- /*
- * Since we dropped the inode lock,
- * the inode may have been reclaimed.
- * Therefore, we reacquire the mount
- * lock and check to see if we were the
- * inode reclaimed. If this happened
- * then the ipointer marker will no
- * longer point back at us. In this
- * case, move ip along to the inode
- * after the marker, remove the marker
- * and continue.
- */
- XFS_MOUNT_ILOCK(mp);
- mount_locked = B_TRUE;
-
- if (ip != ipointer->ip_mprev) {
- IPOINTER_REMOVE(ip, mp);
-
- ASSERT(!vnode_refed);
- ASSERT(!(lock_flags &
- XFS_IOLOCK_SHARED));
- continue;
- }
-
- ASSERT(ip->i_mount == mp);
-
- if (xfs_ilock_nowait(ip,
- XFS_ILOCK_SHARED) == 0) {
- ASSERT(ip->i_mount == mp);
- /*
- * We failed to reacquire
- * the inode lock without
- * sleeping, so just skip
- * the inode for now. We
- * clear the ILOCK bit from
- * the lock_flags so that we
- * won't try to drop a lock
- * we don't hold below.
- */
- lock_flags &= ~XFS_ILOCK_SHARED;
- IPOINTER_REMOVE(ip_next, mp);
- } else if ((xfs_ipincount(ip) == 0) &&
- xfs_iflock_nowait(ip)) {
- ASSERT(ip->i_mount == mp);
- /*
- * Since this is vfs_sync()
- * calling we only flush the
- * inode out if we can lock
- * it without sleeping and
- * it is not pinned. Drop
- * the mount lock here so
- * that we don't hold it for
- * too long. We already have
- * a marker in the list here.
- */
- XFS_MOUNT_IUNLOCK(mp);
- mount_locked = B_FALSE;
- error = xfs_iflush(ip,
- XFS_IFLUSH_DELWRI);
- } else {
- ASSERT(ip->i_mount == mp);
- IPOINTER_REMOVE(ip_next, mp);
- }
- }
-
- }
+ if ((flags & SYNC_ATTR) &&
+ (ip->i_update_core ||
+ (ip->i_itemp && ip->i_itemp->ili_format.ilf_fields))) {
+ if (mount_locked)
+ IPOINTER_INSERT(ip, mp);
- } else {
- if ((flags & SYNC_ATTR) &&
- ((ip->i_update_core) ||
- ((ip->i_itemp != NULL) &&
- (ip->i_itemp->ili_format.ilf_fields != 0)))) {
- if (mount_locked) {
- IPOINTER_INSERT(ip, mp);
- }
+ if (flags & SYNC_WAIT) {
+ xfs_iflock(ip);
+ error = xfs_iflush(ip, XFS_IFLUSH_SYNC);
- if (flags & SYNC_WAIT) {
- xfs_iflock(ip);
- error = xfs_iflush(ip,
- XFS_IFLUSH_SYNC);
- } else {
- /*
- * If we can't acquire the flush
- * lock, then the inode is already
- * being flushed so don't bother
- * waiting. If we can lock it then
- * do a delwri flush so we can
- * combine multiple inode flushes
- * in each disk write.
- */
- if (xfs_iflock_nowait(ip)) {
- error = xfs_iflush(ip,
- XFS_IFLUSH_DELWRI);
- }
- else if (bypassed)
- (*bypassed)++;
- }
+ /*
+ * If we can't acquire the flush lock, then the inode
+ * is already being flushed so don't bother waiting.
+ *
+ * If we can lock it then do a delwri flush so we can
+ * combine multiple inode flushes in each disk write.
+ */
+ } else if (xfs_iflock_nowait(ip)) {
+ error = xfs_iflush(ip, XFS_IFLUSH_DELWRI);
+ } else if (bypassed) {
+ (*bypassed)++;
}
}
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] remove dead SYNC_BDFLUSH case in xfs_sync_inodes
2007-09-09 15:42 [PATCH] remove dead SYNC_BDFLUSH case in xfs_sync_inodes Christoph Hellwig
@ 2007-09-09 19:43 ` Bhagi rathi
2007-09-09 21:37 ` Eric Sandeen
0 siblings, 1 reply; 4+ messages in thread
From: Bhagi rathi @ 2007-09-09 19:43 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
vfs_sync_worker calls with SYNC_BDFLUSH. xfssyncd can call this. I might be
missing something
if this is not used.
Thanks,
-Saradhi.
On 9/9/07, Christoph Hellwig <hch@lst.de> wrote:
>
> A large part of xfs_sync_inodes is conditional on the SYNC_BDFLUSH which
> is never passed to it. This patch removes it and adds an assert that
> triggers in case some new code tries to pass SYNC_BDFLUSH to it.
>
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
>
> Index: linux-2.6-xfs/fs/xfs/xfs_vfsops.c
> ===================================================================
> --- linux-2.6-xfs.orig/fs/xfs/xfs_vfsops.c 2007-09-08 17:43:
> 39.000000000 +0200
> +++ linux-2.6-xfs/fs/xfs/xfs_vfsops.c 2007-09-08 19:06:31.000000000+0200
> @@ -981,8 +981,6 @@ xfs_sync_inodes(
> int *bypassed)
> {
> xfs_inode_t *ip = NULL;
> - xfs_inode_t *ip_next;
> - xfs_buf_t *bp;
> bhv_vnode_t *vp = NULL;
> int error;
> int last_error;
> @@ -992,7 +990,6 @@ xfs_sync_inodes(
> boolean_t mount_locked;
> boolean_t vnode_refed;
> int preempt;
> - xfs_dinode_t *dip;
> xfs_iptr_t *ipointer;
> #ifdef DEBUG
> boolean_t ipointer_in = B_FALSE;
> @@ -1045,6 +1042,8 @@ xfs_sync_inodes(
>
> #define XFS_PREEMPT_MASK 0x7f
>
> + ASSERT(!(flags & SYNC_BDFLUSH));
> +
> if (bypassed)
> *bypassed = 0;
> if (mp->m_flags & XFS_MOUNT_RDONLY)
> @@ -1057,7 +1056,7 @@ xfs_sync_inodes(
> ipointer = (xfs_iptr_t *)kmem_zalloc(sizeof(xfs_iptr_t),
> KM_SLEEP);
>
> fflag = XFS_B_ASYNC; /* default is don't wait */
> - if (flags & (SYNC_BDFLUSH | SYNC_DELWRI))
> + if (flags & SYNC_DELWRI)
> fflag = XFS_B_DELWRI;
> if (flags & SYNC_WAIT)
> fflag = 0; /* synchronous overrides all */
> @@ -1147,24 +1146,6 @@ xfs_sync_inodes(
> }
>
> /*
> - * If this is just vfs_sync() or pflushd() calling
> - * then we can skip inodes for which it looks like
> - * there is nothing to do. Since we don't have the
> - * inode locked this is racy, but these are periodic
> - * calls so it doesn't matter. For the others we want
> - * to know for sure, so we at least try to lock them.
> - */
> - if (flags & SYNC_BDFLUSH) {
> - if (((ip->i_itemp == NULL) ||
> - !(ip->i_itemp->ili_format.ilf_fields &
> - XFS_ILOG_ALL)) &&
> - (ip->i_update_core == 0)) {
> - ip = ip->i_mnext;
> - continue;
> - }
> - }
> -
> - /*
> * Try to lock without sleeping. We're out of order with
> * the inode list lock here, so if we fail we need to drop
> * the mount lock and try again. If we're called from
> @@ -1181,7 +1162,7 @@ xfs_sync_inodes(
> * it.
> */
> if (xfs_ilock_nowait(ip, lock_flags) == 0) {
> - if ((flags & SYNC_BDFLUSH) || (vp == NULL)) {
> + if (vp == NULL) {
> ip = ip->i_mnext;
> continue;
> }
> @@ -1242,160 +1223,27 @@ xfs_sync_inodes(
> xfs_ilock(ip, XFS_ILOCK_SHARED);
> }
>
> - if (flags & SYNC_BDFLUSH) {
> - if ((flags & SYNC_ATTR) &&
> - ((ip->i_update_core) ||
> - ((ip->i_itemp != NULL) &&
> - (ip->i_itemp->ili_format.ilf_fields != 0))))
> {
> -
> - /* Insert marker and drop lock if not
> already
> - * done.
> - */
> - if (mount_locked) {
> - IPOINTER_INSERT(ip, mp);
> - }
> -
> - /*
> - * We don't want the periodic flushing of
> the
> - * inodes by vfs_sync() to interfere with
> - * I/O to the file, especially read I/O
> - * where it is only the access time stamp
> - * that is being flushed out. To prevent
> - * long periods where we have both inode
> - * locks held shared here while reading
> the
> - * inode's buffer in from disk, we drop
> the
> - * inode lock while reading in the inode
> - * buffer. We have to release the buffer
> - * and reacquire the inode lock so that
> they
> - * are acquired in the proper order (inode
> - * locks first). The buffer will go at
> the
> - * end of the lru chain, though, so we can
> - * expect it to still be there when we go
> - * for it again in xfs_iflush().
> - */
> - if ((xfs_ipincount(ip) == 0) &&
> - xfs_iflock_nowait(ip)) {
> -
> - xfs_ifunlock(ip);
> - xfs_iunlock(ip, XFS_ILOCK_SHARED);
> -
> - error = xfs_itobp(mp, NULL, ip,
> - &dip, &bp, 0,
> 0);
> - if (!error) {
> - xfs_buf_relse(bp);
> - } else {
> - /* Bailing out, remove the
> - * marker and free it.
> - */
> - XFS_MOUNT_ILOCK(mp);
> - IPOINTER_REMOVE(ip, mp);
> - XFS_MOUNT_IUNLOCK(mp);
> -
> - ASSERT(!(lock_flags &
> -
> XFS_IOLOCK_SHARED));
> -
> - kmem_free(ipointer,
> -
> sizeof(xfs_iptr_t));
> - return (0);
> - }
> -
> - /*
> - * Since we dropped the inode
> lock,
> - * the inode may have been
> reclaimed.
> - * Therefore, we reacquire the
> mount
> - * lock and check to see if we
> were the
> - * inode reclaimed. If this
> happened
> - * then the ipointer marker will
> no
> - * longer point back at us. In
> this
> - * case, move ip along to the
> inode
> - * after the marker, remove the
> marker
> - * and continue.
> - */
> - XFS_MOUNT_ILOCK(mp);
> - mount_locked = B_TRUE;
> -
> - if (ip != ipointer->ip_mprev) {
> - IPOINTER_REMOVE(ip, mp);
> -
> - ASSERT(!vnode_refed);
> - ASSERT(!(lock_flags &
> -
> XFS_IOLOCK_SHARED));
> - continue;
> - }
> -
> - ASSERT(ip->i_mount == mp);
> -
> - if (xfs_ilock_nowait(ip,
> - XFS_ILOCK_SHARED) ==
> 0) {
> - ASSERT(ip->i_mount == mp);
> - /*
> - * We failed to reacquire
> - * the inode lock without
> - * sleeping, so just skip
> - * the inode for now. We
> - * clear the ILOCK bit
> from
> - * the lock_flags so that
> we
> - * won't try to drop a
> lock
> - * we don't hold below.
> - */
> - lock_flags &=
> ~XFS_ILOCK_SHARED;
> - IPOINTER_REMOVE(ip_next,
> mp);
> - } else if ((xfs_ipincount(ip) ==
> 0) &&
> - xfs_iflock_nowait(ip))
> {
> - ASSERT(ip->i_mount == mp);
> - /*
> - * Since this is
> vfs_sync()
> - * calling we only flush
> the
> - * inode out if we can
> lock
> - * it without sleeping and
> - * it is not pinned. Drop
> - * the mount lock here so
> - * that we don't hold it
> for
> - * too long. We already
> have
> - * a marker in the list
> here.
> - */
> - XFS_MOUNT_IUNLOCK(mp);
> - mount_locked = B_FALSE;
> - error = xfs_iflush(ip,
>
> - XFS_IFLUSH_DELWRI);
> - } else {
> - ASSERT(ip->i_mount == mp);
> - IPOINTER_REMOVE(ip_next,
> mp);
> - }
> - }
> -
> - }
> + if ((flags & SYNC_ATTR) &&
> + (ip->i_update_core ||
> + (ip->i_itemp && ip->i_itemp->ili_format.ilf_fields)))
> {
> + if (mount_locked)
> + IPOINTER_INSERT(ip, mp);
>
> - } else {
> - if ((flags & SYNC_ATTR) &&
> - ((ip->i_update_core) ||
> - ((ip->i_itemp != NULL) &&
> - (ip->i_itemp->ili_format.ilf_fields != 0))))
> {
> - if (mount_locked) {
> - IPOINTER_INSERT(ip, mp);
> - }
> + if (flags & SYNC_WAIT) {
> + xfs_iflock(ip);
> + error = xfs_iflush(ip, XFS_IFLUSH_SYNC);
>
> - if (flags & SYNC_WAIT) {
> - xfs_iflock(ip);
> - error = xfs_iflush(ip,
>
> - XFS_IFLUSH_SYNC);
> - } else {
> - /*
> - * If we can't acquire the flush
> - * lock, then the inode is already
> - * being flushed so don't bother
> - * waiting. If we can lock it
> then
> - * do a delwri flush so we can
> - * combine multiple inode flushes
> - * in each disk write.
> - */
> - if (xfs_iflock_nowait(ip)) {
> - error = xfs_iflush(ip,
>
> - XFS_IFLUSH_DELWRI);
> - }
> - else if (bypassed)
> - (*bypassed)++;
> - }
> + /*
> + * If we can't acquire the flush lock, then the
> inode
> + * is already being flushed so don't bother
> waiting.
> + *
> + * If we can lock it then do a delwri flush so we
> can
> + * combine multiple inode flushes in each disk
> write.
> + */
> + } else if (xfs_iflock_nowait(ip)) {
> + error = xfs_iflush(ip, XFS_IFLUSH_DELWRI);
> + } else if (bypassed) {
> + (*bypassed)++;
> }
> }
>
>
>
>
[[HTML alternate version deleted]]
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] remove dead SYNC_BDFLUSH case in xfs_sync_inodes
2007-09-09 19:43 ` Bhagi rathi
@ 2007-09-09 21:37 ` Eric Sandeen
2007-09-10 4:54 ` Bhagi rathi
0 siblings, 1 reply; 4+ messages in thread
From: Eric Sandeen @ 2007-09-09 21:37 UTC (permalink / raw)
To: Bhagi rathi; +Cc: Christoph Hellwig, xfs
Bhagi rathi wrote:
> vfs_sync_worker calls with SYNC_BDFLUSH. xfssyncd can call this. I might be
> missing something
> if this is not used.
>
> Thanks,
> -Saradhi.
My eyes glazed over it too, but in xfs_syncsub as hch pointed out to me:
if (flags & (SYNC_ATTR|SYNC_DELWRI)) {
if (flags & SYNC_BDFLUSH)
xfs_finish_reclaim_all(mp, 1);
else
error = xfs_sync_inodes(mp, flags, bypassed);
}
so it won't be called with that flag.
-Eric
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] remove dead SYNC_BDFLUSH case in xfs_sync_inodes
2007-09-09 21:37 ` Eric Sandeen
@ 2007-09-10 4:54 ` Bhagi rathi
0 siblings, 0 replies; 4+ messages in thread
From: Bhagi rathi @ 2007-09-10 4:54 UTC (permalink / raw)
To: Eric Sandeen; +Cc: Christoph Hellwig, xfs
Ah, I see. It looks we don't use SYNC_BDFLUSH in xfs_sync_inodes. I believe
we don't
want to flush inodes from xfssync as inodes will be flushed by pdflush,
memory cleaner
threads. It looks like this code may be valid to other operating systems,
not for linux?
-Saradhi.
On 9/10/07, Eric Sandeen <sandeen@sandeen.net> wrote:
>
> Bhagi rathi wrote:
> > vfs_sync_worker calls with SYNC_BDFLUSH. xfssyncd can call this. I might
> be
> > missing something
> > if this is not used.
> >
> > Thanks,
> > -Saradhi.
>
>
> My eyes glazed over it too, but in xfs_syncsub as hch pointed out to me:
>
> if (flags & (SYNC_ATTR|SYNC_DELWRI)) {
> if (flags & SYNC_BDFLUSH)
> xfs_finish_reclaim_all(mp, 1);
> else
> error = xfs_sync_inodes(mp, flags, bypassed);
> }
>
> so it won't be called with that flag.
>
> -Eric
>
[[HTML alternate version deleted]]
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2007-09-10 4:55 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-09-09 15:42 [PATCH] remove dead SYNC_BDFLUSH case in xfs_sync_inodes Christoph Hellwig
2007-09-09 19:43 ` Bhagi rathi
2007-09-09 21:37 ` Eric Sandeen
2007-09-10 4:54 ` Bhagi rathi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox