[Top] [All Lists]

Re: [PATCH] xfs: pull up stack_switch check into xfs_bmapi_write

To: Brian Foster <bfoster@xxxxxxxxxx>
Subject: Re: [PATCH] xfs: pull up stack_switch check into xfs_bmapi_write
From: Mark Tinguely <tinguely@xxxxxxx>
Date: Thu, 17 Jan 2013 12:50:55 -0600
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <1358446289-871-1-git-send-email-bfoster@xxxxxxxxxx>
References: <1358446289-871-1-git-send-email-bfoster@xxxxxxxxxx>
User-agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:9.0) Gecko/20120122 Thunderbird/9.0
On 01/17/13 12:11, Brian Foster wrote:
The stack_switch check currently occurs in __xfs_bmapi_allocate,
which means the stack switch only occurs when xfs_bmapi_allocate()
is called in a loop. Pull the check up before the loop in
xfs_bmapi_write() such that the first iteration of the loop has
consistent behavior.

Signed-off-by: Brian Foster<bfoster@xxxxxxxxxx>

I was reading through this code and confused myself over whether the stack
switch ever actually occurs. Eric and Ben pointed out on irc (simultaneously,
I might add) the surrounding loop that I had missed, but it wasn't clear whether
the behavior to enable the stack switch after the first iteration was
intentional or not. I'm throwing this out there to either fix the issue or seek
out an explanation for the existing behavior. Thanks!


  fs/xfs/xfs_bmap.c |    6 +++---
  1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/xfs_bmap.c b/fs/xfs/xfs_bmap.c
index c507720..491f35e 100644
--- a/fs/xfs/xfs_bmap.c
+++ b/fs/xfs/xfs_bmap.c
@@ -4676,9 +4676,6 @@ __xfs_bmapi_allocate(
                        return error;

-       if (bma->flags&  XFS_BMAPI_STACK_SWITCH)
-               bma->stack_switch = 1;
        error = xfs_bmap_alloc(bma);
        if (error)
                return error;
@@ -4952,6 +4949,9 @@ xfs_bmapi_write(
        bma.flist = flist;
        bma.firstblock = firstblock;

+       if (flags&  XFS_BMAPI_STACK_SWITCH)
+               bma.stack_switch = 1;
        while (bno<  end&&  n<  *nmap) {
                inhole = eof || bma.got.br_startoff>  bno;
                wasdelay = !inhole&&  isnullstartblock(bma.got.br_startblock);

Might want to read the Sep 2012 OSS archives, for example the below thread and the spin off threads.


My original patch was in xfs_bmapi_write() and it was moved to its current location.

You can force the code by restricting to only one allocation worker and there were 2 xfstests (76? and 139? come to mind) that quickly triggered the event.


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