Bug 231380

On the node running iozone, it looks like its doing the "right thing" in that
its obviously received a callback for the lock that its using and its trying to
write out the dirty data so that it can then release the lock. It appears to be
stuck obtaining the page lock for the page which is a bit strange since I can't
see what else might be holding that lock.

On the node running du, the situation is far from clear. That backtrace looks
very odd to me as it seems to contain a jumble of information from different
syscalls. It does look at though its trying to read xattrs which I presume is
what is causing the request for the lock from the other node, since the node
running du does appear to be awaiting a glock being released.

So the problem is most likely to be on the node running iozone since thats the
one which appears to not be releasing the lock in question. We need to look at
what else on that node might be holding the page lock.

well i thought that we may not be unlocking the page in gfs2_writepage (idk 
why b/c i couldn't really see a situation in block_write_full_page where the 
page wouldn't be unlocked) and my printk didn't get tripped so we are 
unlocking the page in gfs2_writepage, so the problem has to be happening 
somewhere after that.

running a really stupid debug patch, it looks more like we are looping much 
like bonnie++ did with gfs2_prepare_write, the only reason I'm not getting 
softlockups is because the box that is looping is a multiprocessor box.  I'm 
going to look into why this might be happening.

hmm or not, I'm going to try and rework the way I'm thinking about this 
problem.

hmm so i think i've figured out the problem, but i'm kind of unsure how to fix 
this properly.  So we come in through generic_file_buffered_write and get our 
page through __grab_cache_page, which comes back with the page locked.  We 
then come into gfs2_commit_write, which does its thing and then calls 
gfs2_glock_dq_m.  Since we have somebody waiting for that lock we go through 
to flush out all dirty pages, and then hang on lock_page(), because we have 
the page still locked from generic_file_buffered_write.  So this is what I'm 
thinking, around the gfs2_glock_dq_m putting a unlock_page/lock_page to keep 
this problem from happening.  I'm going to build a test kernel and see if that 
resolves the problem, but I would like to know if thats the best way to be 
going about this problem.

Created attachment 149867 [details]
patch that resolves the problem.

ok this withstands my horrible iozone/du testing and fixes the problem nicely. 
I'm going to post to cluster-devel to see if anybody has a better idea on how
to fix this.

hmm i'm thinking this may be kind of a performance killer in the case that we
don't actually have anybody waiting on the glock.  Instead of just blindly
unlocking/locking the page, instead check to see if we have waiters, and if we
do unlock/lock the page, else just do what we normally do.  Of course we could
race and possibly get a waiter in the middle of our deque operation.

I've just been looking at the OCFS2 code and I notice that they do not appear to
have had this problem as their code has no obvious solution for it. I agree that
the unlock and relock of the page is nasty, on the other hand I cannot see any
reason why this shouldn't be correct.

You can test for a waiter with test_bit(GLF_DEMOTE, &gl->gl_flags) - this
assumes that you have the GLF_DEMOTE patch - but there will be a race in doing
that since it can be set at any time that the gl->gl_spin lock isn't held, so I
suspect that unlocking and relocking is the only option for now.

Another approach would be to make the unlock operation ignore any pending
waiting lock/demote requests, however there is then the question of when these
requests should then run, so that seems to just introduce another problem.
Eventually we will need to tackle that problem too when we come to look at
preventing lock bounce, but I was hoping not to need to deal with that at this
stage.

Also, although you unlock the page, when the glops functions run due to the
demote request we need to check that the page will be marked "not uptodate" and
not just ignored since it will have an elevated ref count due to the VFS holding
a ref still at this stage. This may potentially have some bearing on the other
bug, bz #231910.

how about this.  we make a sort of "fast unlock" for these kind of situations,
where we know we will likely be coming back and reclaiming the lock again
anyway.  In the "fast unlock" we just don't do anything with anybody who is
waiting on us, and instead we have a kernel thread that sits there and every
tunable amount of seconds runs through the list of glocks that have waiters and
do the normal operations you do on a glock if it has waiters and then let them
have the glock.  This way we don't have to worry about this problem, even if we
get to that area where we are between the lock and gfs2_commit_write, the page
shouldn't be dirty because it hasn't yet been committed so theres no chance of a
race (i think).  Course this has the downside of making other nodes wait longer
for a lock when an exclusive lock is held, but I'm thinking this is the only
case where this would be a problem.

Then again if this is the only place where this is a problem it would be kind of
crappy to put a kernel thread in just to make sure this section doesn't lock up.
 IIRC the GLF_DEMOTE flag is cleared as soon as you do the rq_demote() stuff
when you are dequeing the glock.  We could instead move that into the enque
code.  So we grab the lock, then if we have GLF_DEMOTE set, make sure all dirty
pages associated with that inode are flushed and then clear GLF_DEMOTE.  But
then if the glock is taken from another node who has dirty pages for that inode
I'm not really sure how you go about making that happen.

So all in all I have no idea how to fix this :)

The "fast unlock" as you call it is something we'll need eventually anyway for a
variety of reasons, but we still need to be able to release the lock from here.
I'm hoping that the OCFS2 plan to have a single call in which to wrap both
prepare_write and commit_write will eventually solve this for us by allowing us
to get the locks in the right order, so we only really need a temporary fix to
this right now. So far I've not been able to work out anything better than your
original suggestion, provided we can be sure of all the pages being invalidated,
which is something I'm looking at now.

The "OCFS2 plan" I referred to in comment #10 is progressing in the form of
patches which have just been posted for review by Nick Piggin on lkml and the
subject lines are:

Re: [patch 2/3] fs: introduce perform_write aop
[patch 1/5] fs: add an iovec iterator
[patch 2/5] fs: introduce new aops and infrastructure
[patch 3/5] fs: convert some simple filesystems
[patch 4/5] ext2: convert to new aops
[patch 5/5] ext3: convert to new aops
Re: [patch 1/5] fs: add an iovec iterator

This won't be a possible solution in RHEL5 since its too invasive, but upstream
should eventually benefit from it.

Rob K, please add some ACKs to this one. 

This request was evaluated by Red Hat Kernel Team for inclusion in a Red
Hat Enterprise Linux maintenance release, and has moved to bugzilla 
status POST.

in 2.6.18-15.el5

An advisory has been issued which should help the problem
described in this bug report. This report is therefore being
closed with a resolution of ERRATA. For more information
on the solution and/or where to find the updated files,
please follow the link below. You may reopen this bug report
if the solution does not work for you.

http://rhn.redhat.com/errata/RHBA-2007-0959.html