Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751241AbcDFQLB (ORCPT ); Wed, 6 Apr 2016 12:11:01 -0400 Received: from mail-io0-f193.google.com ([209.85.223.193]:34366 "EHLO mail-io0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750731AbcDFQK7 (ORCPT ); Wed, 6 Apr 2016 12:10:59 -0400 MIME-Version: 1.0 Reply-To: fdmanana@gmail.com In-Reply-To: <1459958196-31346-1-git-send-email-bastienphilbert@gmail.com> References: <1459958196-31346-1-git-send-email-bastienphilbert@gmail.com> Date: Wed, 6 Apr 2016 17:10:58 +0100 Message-ID: Subject: Re: [PATCH] btrfs:Change BUG_ON to new error path in __clear_extent_bit From: Filipe Manana To: Bastien Philbert Cc: Chris Mason , Josef Bacik , David Sterba , "linux-btrfs@vger.kernel.org" , "linux-kernel@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2478 Lines: 68 On Wed, Apr 6, 2016 at 4:56 PM, Bastien Philbert wrote: > This remove the unnessary BUG_ON if the allocation with > alloc_extent_state_atomic fails due to this function > failure not being unrecoverable. Instead we now change > this BUG_ON into a new error path that jumps to the goto > label, out from freeing previously allocated resources > before returning the error code -ENOMEM to signal callers > that the call to __clear_extent_bit failed due to a memory > allocation failure. > > Signed-off-by: Bastien Philbert > --- > fs/btrfs/extent_io.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c > index d247fc0..4c87b77 100644 > --- a/fs/btrfs/extent_io.c > +++ b/fs/btrfs/extent_io.c > @@ -682,7 +682,10 @@ hit_next: > > if (state->start < start) { > prealloc = alloc_extent_state_atomic(prealloc); > - BUG_ON(!prealloc); > + if (!prealloc) { > + err = -ENOMEM; > + goto out; > + } Why only in this particular place? We do this in other places in this function. Second, setting err to -ENOMEM is not enough. Under the out label we always return 0, so you're not propagating the error to callers. Now, most importantly, you didn't check if callers handle errors from this function (__clear_extent_bit()) at all. A failure in this function is critical. For example, it can cause a range in an inode's io tree to become locked forever, blocking any other tasks that want to operate on the range, and we won't ever know what happened. So it's far from trivial to handle errors from this function and that's why the BUG_ON is there. If you really want to get rid of the BUG_ON() calls you need to make sure all callers don't ignore the errors and that they deal with them properly. > err = split_state(tree, state, prealloc, start); > if (err) > extent_io_tree_panic(tree, err); > -- > 2.5.0 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Filipe David Manana, "Reasonable men adapt themselves to the world. Unreasonable men adapt the world to themselves. That's why all progress depends on unreasonable men."