On Mon, 2010-11-08 at 09:15 -0500, Josef Bacik wrote: > On Mon, Nov 08, 2010 at 10:06:13PM +0800, Ian Kent wrote: > > On Mon, 2010-11-08 at 07:42 -0500, Josef Bacik wrote: > > > On Mon, Nov 08, 2010 at 10:54:07AM +0800, Ian Kent wrote: > > > > On Sun, 2010-11-07 at 09:51 -0500, Josef Bacik wrote: > > > > > On Sun, Nov 07, 2010 at 04:16:47PM +0900, Yoshinori Sano wrote: > > > > > > This is a question I've posted on the #btrfs IRC channel today. > > > > > > hyperair adviced me to contact with Josef Bacik or Chris Mason. > > > > > > So, I post my question to this maling list. > > > > > > > > > > > > Here are my post on the IRC: > > > > > > > > > > > > Actually, I want to remove BUG_ON(ret) around the Btrfs code. > > > > > > The motivation is to make the Btrfs code more robust. > > > > > > First of all, is this meaningless? > > > > > > > > > > > > For example, there are code like the following: > > > > > > > > > > > > struct btrfs_path *path; > > > > > > path = btrfs_alloc_path(); > > > > > > BUG_ON(!path); > > > > > > > > > > > > This is a frequenty used pattern of current Btrfs code. > > > > > > A btrfs_alloc_path()'s caller has to deal with the allocation failure > > > > > > instead of using BUG_ON. However, (this is what most interesting > > > > > > thing for me) can the caller do any proper error handlings here? > > > > > > I mean, is this a critical situation where we cannot recover from? > > > > > > > > > > > > > > > > No we're just lazy ;). Tho making sure the caller can recover from getting > > > > > -ENOMEM is very important, which is why in some of these paths we just do BUG_ON > > > > > since fixing the callers is tricky. A good strategy for things like this is to > > > > > do something like > > > > > > > > > > static int foo = 1; > > > > > > > > > > path = btrfs_alloc_path(); > > > > > if (!path || !(foo % 1000)) > > > > > return -ENOMEM; > > > > > foo++; > > > > > > > > Hahaha, I love it. > > > > > > > > So, return ENOMEM every 1000 times we call the containing function! > > > > > > > > > > > > > > that way you can catch all the callers and make sure we're handling the error > > > > > all the way up the chain properly. Thanks, > > > > > > > > Yeah, I suspect this approach will be a bit confusing though. > > > > > > > > I believe that it will be more effective, although time consuming, to > > > > work through the call tree function by function. Although, as I have > > > > said, the problem is working out what needs to be done to recover, > > > > rather than working out what the callers are. I'm not at all sure yet > > > > but I also suspect that it may not be possible to recover in some cases, > > > > which will likely lead to serious rework of some subsystems (but, hey, > > > > who am I to say, I really don't have any clue yet). > > > > > > > > > > So we talked about this at plumbers. First thing we need is a way to flip the > > > filesystem read only, that way we can deal with the simple corruption cases. > > > > Right, yes. > > > > > And then we can start looking at these harder cases where it's really unclear > > > about how to recover. > > > > I have a way to go before I will even understand these cases. > > > > > > > > Thankfully because we're COW we really shouldn't have any cases that we have to > > > unwind anything, we just fail the operation and go on our happy merry way. The > > > only tricky thing is where we get ENOMEM when say inserting the metadata for > > > data after writing out the data, since that will leave data just sitting around. > > > Probably should look at what NFS does with dirty pages when the server hangs up. > > > > OK, that's a though for me to focus on while I'm trying to work out > > what's going on ... mmm. > > > > Indeed, a large proportion of these are handling ENOMEM. > > > > I somehow suspect your heavily focused on disk io itself when I'm still > > back thinking about house keeping of operations, in the process of being > > queued and those currently being processed, the later being the > > difficult case. But I'll eventually get to worrying about io as part of > > that process. It's also worth mentioning that my scope is also quite > > narrow at this stage, focusing largely on the transaction subsystem, > > although that tends to pull in a fair amount of other code too. > > > > So the transaction stuff should be relatively simple since we shouldn't have too > much to clean up if the transaction fails to allocate. Maybe point out some > places where you are having trouble and I can frame up what we'd want to do to > give you an idea of where to go? Thanks, Thanks, I will when I have something to discuss or maybe I'll start with the couple I have, when I get a chance to get back to it anyway. Ian -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
