From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tao Ma Date: Fri, 24 Oct 2008 09:17:40 +0800 Subject: [Ocfs2-devel] [PATCH 2/3] ocfs2/xattr: Merge xattr set transaction. In-Reply-To: <20081024010912.GG12751@mail.oracle.com> References: <48F81730.9020809@oracle.com> <1224218678-32196-2-git-send-email-tao.ma@oracle.com> <20081023214036.GC12751@mail.oracle.com> <49011A69.9040003@oracle.com> <20081024010912.GG12751@mail.oracle.com> Message-ID: <49012234.7050807@oracle.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: ocfs2-devel@oss.oracle.com Joel Becker wrote: > On Fri, Oct 24, 2008 at 08:44:25AM +0800, Tao Ma wrote: >> Joel Becker wrote: >>>> fs/ocfs2/xattr.c | 889 +++++++++++++++++++++++++++++++----------------------- >>>> 1 files changed, 514 insertions(+), 375 deletions(-) >>> I'm trying to think of a way you could break this patch up. >>> It's huge. >> Actually, most of the modification are just changing ocfs2_start_trans >> to ocfs2_extend_trans and passing 'handle_t *' to the function. So >> although the patch is a little large, but I think it is easy to review, >> does it? ;) > > No, I found it incredibly hard to wade through, because I'd get > to a point where I'd lost track of where I was, and I would look and see > I'd only viewed 44% of the patch :-) > There's a lot going on in here. You're moving the transaction > calculations around, moving locks, reorganizing the calls to the various > set handlers, and that's just for starters. OK, I will try to separate it into small ones. > >>>> static int ocfs2_xattr_set_entry_index_block(struct inode *inode, >>>> + handle_t *handle, >>>> struct ocfs2_xattr_info *xi, >>>> - struct ocfs2_xattr_search *xs); >>>> + struct ocfs2_xattr_search *xs, >>>> + struct ocfs2_xattr_set_ctxt *ctxt); >>> You pass 'handle' and 'ctxt' to all the same functions. Why not >>> put the handle on the ctxt? >> Most functions doesn't need 'ctxt' actually, only the functions which >> use cluster/metadata allocation/free use it. So I'd like to separate >> them. > > Almost every function I saw that took 'handle' also took 'ctxt'. > But that's not such a big deal. > >>>> + clusters_to_add) + handle->h_buffer_credits; >>>> + status = ocfs2_extend_trans(handle, credits); >>> Um, you just made 'credits' be double the existing transaction + >>> the result of ocfs2_calc_extend_credits(). Did you mean that? >>> ocfs2_extend_trans() takes the additional credits, not the total. Later >>> you do an eocfs2_extend_trans() with merely the new credits, so I wonder >>> if you meant this here? >> Yes, I do this intentionally. Because ocfs2_extend_trans sometimes >> doesn't work like its name. ;) If the first extension fails, it will >> commit the dirty blocks and then extend it, that make the credits which >> isn't used lost. In some cases it is OK(see some of my following >> modification), while in other cases when some metadata will be updated >> after this function, it will fails because of no credits. I have once >> meet with a bug for this. And actually tree operation do the same thing >> as I do. See ocfs2_extend_rotate_transaction. > > Ok, so the basic thought is that we're going to need > h_buffer_credits after we return from this function in other places - > we're not done with them just because we commit inside > ocfs2_extend_trans(). > The big concern here is that: > > 1) ocfs2_extend_trans() commits the half-completed work so it can > free up some space in the journal. > 2) The system crashes. > 3) jbd2 replays the half-completed work. > 4) We mount again, and our code can't handle the half-completed work. > > The alloc.c code is written to handle this half-completed state > of our trees. Actually, our trees almost never end up in half-completed > states. They are always valid trees at the point we might extend/commit > a transaction. I'm not worried about the xattr trees (index or value) > in this regard. I'm worried about any fields we've updated that get > written due to these unexpected commits. > For example, when setting an xattr you try ibody_set() first. > If that succeeds, you then go ahead and clear the same xattr out of the > external block/tree. So if there is an extend_trans() somewhere in > there, and it commits a change, you could crash and have the xattr in > both the ibody and the external block/tree. The same with the reverse > (set in external, commit, clear in ibody, crash). We need to make sure > we either don't actually have these cases happen or handle them > gracefully. yeah, you may be right. As my previous e-mail said, I will try to calculate all the credits in the beginning. Let's see how it works. Regards, Tao