Btrfs devel - Apr 2013 - [PATCH] btrfs: abort unlink trans in missed error case

__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(+)

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)
-- 
1.8.1.4

--
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/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