| Message ID | 1364936536-13130-1-git-send-email-zab@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 4/2/13 4:02 PM, Zach Brown wrote: > __btrfs_unlink_inode() aborts its transaction when it sees errors after > it removes the directory item. But it missed the case where > btrfs_del_dir_entries_in_log() returns an error. If this happens then > the unlink appears to fail but the items have been removed without > updating the directory size. The directory then has leaked bytes in > i_size and can never be removed. > > Adding the missing transaction abort at least makes this failure > consistent with the other failure cases. > > I noticed this while reading the code after someone on irc reported > having a directory with i_size but no entries. I tested it by forcing > btrfs_del_dir_entries_in_log() to return -ENOMEM. I was wondering if the transaction support should just be in the err: goto case, and went looking. I'm not familiar enough with this stuff yet, but what if i.e. btrfs_delete_one_dir_name fails, should that also abort the transaction? > Signed-off-by: Zach Brown <zab@redhat.com> > --- > fs/btrfs/inode.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > index d96ee30..80676ee 100644 > --- a/fs/btrfs/inode.c > +++ b/fs/btrfs/inode.c > @@ -3619,6 +3619,8 @@ static int __btrfs_unlink_inode(struct btrfs_trans_handle *trans, > dir, index); > if (ret == -ENOENT) > ret = 0; > + else if (ret) > + btrfs_abort_transaction(trans, root, ret); > err: > btrfs_free_path(path); > if (ret) > -- 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
> > I was wondering if the transaction support should just be in the > err: goto case, and went looking. Yeah, it's tempting. In the end I decided against it because this shouldn't be so willing to freak out and make the file system read only. It should try and undo the partial unlink and if *that* fails it should go read only. I went for the minimal fix for now. > I'm not familiar enough with this stuff yet, but what if i.e. > btrfs_delete_one_dir_name fails, should that also abort the > transaction? It doesn't abort because its the first thing that can fail. It can cleanly return an error without leaving partial state around. - z -- 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
On 4/3/13 11:04 AM, Zach Brown wrote: >> >> I was wondering if the transaction support should just be in the >> err: goto case, and went looking. > > Yeah, it's tempting. In the end I decided against it because this > shouldn't be so willing to freak out and make the file system read only. > It should try and undo the partial unlink and if *that* fails it should > go read only. I went for the minimal fix for now. > >> I'm not familiar enough with this stuff yet, but what if i.e. >> btrfs_delete_one_dir_name fails, should that also abort the >> transaction? > > It doesn't abort because its the first thing that can fail. It can > cleanly return an error without leaving partial state around. > > - z > Oh, sure. thanks - Reviewed-by: Eric Sandeen <sandeen@redhat.com> -- 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
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index d96ee30..80676ee 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -3619,6 +3619,8 @@ static int __btrfs_unlink_inode(struct btrfs_trans_handle *trans, dir, index); if (ret == -ENOENT) ret = 0; + else if (ret) + btrfs_abort_transaction(trans, root, ret); err: btrfs_free_path(path); if (ret)
__btrfs_unlink_inode() aborts its transaction when it sees errors after it removes the directory item. But it missed the case where btrfs_del_dir_entries_in_log() returns an error. If this happens then the unlink appears to fail but the items have been removed without updating the directory size. The directory then has leaked bytes in i_size and can never be removed. Adding the missing transaction abort at least makes this failure consistent with the other failure cases. I noticed this while reading the code after someone on irc reported having a directory with i_size but no entries. I tested it by forcing btrfs_del_dir_entries_in_log() to return -ENOMEM. Signed-off-by: Zach Brown <zab@redhat.com> --- fs/btrfs/inode.c | 2 ++ 1 file changed, 2 insertions(+)