On 27.09.2017 17:00, David Sterba wrote:
> On Wed, Sep 27, 2017 at 11:48:59AM +0300, Nikolay Borisov wrote:
>> On 26.09.2017 20:39, David Sterba wrote:
>>> On Tue, Sep 26, 2017 at 05:27:41PM +0300, Nikolay Borisov wrote:
>>>> btrfs_update_root can fail for any number of reasons, however the only
>>>> error
>>>> handling we do is to revert the modified flags, yet we do not abort the
>>>> transaction but proceed to commit it.
>>>
>>> AFAICS btrfs_update_root aborts the transaction itself, so it's not
>>> needed in the caller.
>>>
>>>> Fix this by explicitly checking if the
>>>> update root routine has failed and abort the transaction.
>>>>
>>>> Fixes: 0caa102da827 ("Btrfs: Add BTRFS_IOC_SUBVOL_GETFLAGS/SETFLAGS
>>>> ioctls")
>>>> Signed-off-by: Nikolay Borisov <nbori...@suse.com>
>>>> ---
>>>> fs/btrfs/ioctl.c | 3 +++
>>>> 1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>>>> index d6715c2bcdc4..09fcd51f0e8c 100644
>>>> --- a/fs/btrfs/ioctl.c
>>>> +++ b/fs/btrfs/ioctl.c
>>>> @@ -1842,8 +1842,11 @@ static noinline int
>>>> btrfs_ioctl_subvol_setflags(struct file *file,
>>>>
>>>> ret = btrfs_update_root(trans, fs_info->tree_root,
>>>> &root->root_key, &root->root_item);
>>>> + if (ret < 0)
>>>> + btrfs_abort_transaction(trans, ret);
>>>>
>>>> btrfs_commit_transaction(trans);
>>>
>>> I think the error from commit_transaction should be returned as well if
>>> we get here.
>>>
>>> So:
>>>
>>> ret = btrfs_update_root();
>>>
>>> if (ret < 0) {
>>> end_transaction();
>>> goto out_reset;
>>> }
>>>
>>> ret = btrfs_commit_transaction();
>>>
>>> out_reset:
>>> /* and then as it is */
>>> if (ret)
>>> btrfs_set_root_flags(...);
>>>
>>> etc.
>>
>> I think even this is not needed, since btrfs_commit_transaction actually
>> handles aborted transaction by calling btrfs_end_transaction and
>> returning the error with which the trans was aborted. So I will just
>> drop this patch
>
> From API point of view, I'd like to see some pattern, where we don't
> call commit after transaction abort IF we have enough information from
> the context. Ie. the abort and commit would be in the same function.
> Commit has to deal with an aborted transaction, that's right, but kind
> of an implementation detail and last resort resolution.
Fair point, I will respin the patch as well as some of my other
transaction error handling patches.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
