[Cluster-devel] [PATCH 4 of 5] Bz #248176: GFS2: invalid metadata block - REVISED

[Steven Whitehouse <swhiteho@redhat.com>]

Hi,

On Fri, 2007-08-10 at 09:12 -0400, Wendy Cheng wrote:
> Steven Whitehouse wrote:
> 
> >Hi,
> >
> >On Thu, 2007-08-09 at 14:21 -0400, Wendy Cheng wrote:
> >  
> >
> >>Bob Peterson wrote:
> >>    
> >>
> >>>On Thu, 2007-08-09 at 09:46 -0400, Wendy Cheng wrote:
> >>>  
> >>>      
> >>>
> >>>>Set aside "after this patch, the problem goes away" thing ...
> >>>>
> >>>>I haven't checked previous three patches yet so I may not have the 
> >>>>overall picture ... but why adding the journal flush spin lock here 
> >>>>could prevent the new inode to get re-used before its associated buffer 
> >>>>are flushed to the logs ? Could you elaborate more ?
> >>>>
> >>>>    
> >>>>        
> >>>>
> >>>>>+		down_write(&sdp->sd_log_flush_lock);
> >>>>> 		block = rgblk_search(rgd, goal, GFS2_BLKST_UNLINKED,
> >>>>> 				     GFS2_BLKST_UNLINKED);
> >>>>>+		up_write(&sdp->sd_log_flush_lock);
> >>>>>      
> >>>>>          
> >>>>>
> >>>IIRC, if we don't protect rgblk_search from finding GFS2_BLKST_UNLINKED
> >>>blocks, a "deleted" inode may be returned to function
> >>>gfs2_inplace_reserve_i which will do an iput on the inode,
> >>>which may reference buffers that are being flushed to disk.
> >>>If almost all blocks in that bitmap are allocated, I think the
> >>>deleted block may sometimes be reused and the buffer 
> >>>associated with the reused block may be changed before it's
> >>>actually written out to disk.
> >>>  
> >>>      
> >>>
> >>Log flushing is an asynchronous event. I still don't see how this can 
> >>*protect* the condition you just described (i.e., prevents the block 
> >>being assigned to someone else before log flush occurs).  Or do I 
> >>understand your statement right (i.e., the log flushing must occur 
> >>before the block is used by someone else) ? It may *reduce* the 
> >>possibility (if log flushing happens at the same time as this 
> >>assignment) but I don't see how it can *stop* the condition.
> >>
> >>You may "reduce" the (rare) possibility but the real issue is still 
> >>hanging there ? If this is true, then I don't agree we have to pay the 
> >>price of moving a journal flushing lock into resource handling code.
> >>
> >>-- Wendy
> >>
> >>    
> >>
> >
> >Due to the way in which the locking is defined, the journal lock is also
> >used to keep other processes out of the rgrp bitmaps. This prevents the
> >state of the rgrp bitmaps changing while we are scanning them in case a
> >journal flush might occur.
> >
> >The sd_log_flush_lock is an rwsem which is held in read mode by each and
> >every transaction and in write mode when flushing the journal. Log
> >flushing ought to be an asynchronous event, but due to the design of the
> >journaling, it unfortunately isn't in GFS2. It is something that we need
> >to review in the future,
> >
> >
> >  
> >
> 
> It is still not clear what exactly does this lock protect ? The unlinked 
> rgrp bitmap itself or the buffers associated with these disk blocks ? If 
> it is later (the buffers as Bob said),  it implies for every block GFS2 
> takes from this unlinked bitmap, journal flush *has* to happen before it 
> can be used ? Could you elaborate more ?
> 
> -- Wendy
> 
A journal flush is required in order for blocks which have been freed
during the current transaction to become visible to the rest of the
filesystem again. We have two sets of bitmaps, the "normal" set and the
"clone" set. The "normal" set is what we read off disk and what we use
to allocate blocks from.

The "clone" set are created as an exact copy of the "normal" set if (and
only if) we try to deallocate some blocks. In that case the allocation
operation occurs in both bitmaps while the clone exists. When the
journal is flushed, the clone bitmap is copied back into the normal
bitmap for the rgrp, thus making the freed blocks available to the
filesystem for allocation in the following transactions.

When we are looking for unlinked, but not yet deallocated inodes to
free, we need to check the clone bitmap since thats where we mark the
inode free. If we don't do that we might try to free the inode twice
(bug #1 which this patch solves). The other problem is that the locking
governing when the clone bitmap is written back into the normal bitmap
is the journal flush lock (as per the last email) and we have to hold it
to avoid a journal flush from changing the bitmap as we are scanning it.

Steve.