* [PATCH] Btrfs: do not abort transaction when failing to insert hole extent
@ 2018-03-30 22:11 Liu Bo
2018-04-05 16:48 ` David Sterba
0 siblings, 1 reply; 8+ messages in thread
From: Liu Bo @ 2018-03-30 22:11 UTC (permalink / raw)
To: linux-btrfs
This is running in a typical write path, not inside a critical path
where we have to abort the running transaction, so it's OK to return
errors to callers and eventually to userspace.
Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
---
fs/btrfs/inode.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index c7b75dd..b9310f8 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -4939,16 +4939,13 @@ static int maybe_insert_hole(struct btrfs_root *root, struct inode *inode,
ret = btrfs_drop_extents(trans, root, inode, offset, offset + len, 1);
if (ret) {
- btrfs_abort_transaction(trans, ret);
btrfs_end_transaction(trans);
return ret;
}
ret = btrfs_insert_file_extent(trans, root, btrfs_ino(BTRFS_I(inode)),
offset, 0, 0, len, 0, len, 0, 0, 0);
- if (ret)
- btrfs_abort_transaction(trans, ret);
- else
+ if (!ret)
btrfs_update_inode(trans, root, inode);
btrfs_end_transaction(trans);
return ret;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] Btrfs: do not abort transaction when failing to insert hole extent
2018-03-30 22:11 [PATCH] Btrfs: do not abort transaction when failing to insert hole extent Liu Bo
@ 2018-04-05 16:48 ` David Sterba
2018-04-05 18:58 ` Liu Bo
0 siblings, 1 reply; 8+ messages in thread
From: David Sterba @ 2018-04-05 16:48 UTC (permalink / raw)
To: Liu Bo; +Cc: linux-btrfs
On Sat, Mar 31, 2018 at 06:11:55AM +0800, Liu Bo wrote:
> This is running in a typical write path, not inside a critical path
> where we have to abort the running transaction, so it's OK to return
> errors to callers and eventually to userspace.
I'm not sure this is entierly correct, several other places do not abort
after btrfs_drop_extents as there's nothing that would leave the
structres in some half-state.
> Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
> ---
> fs/btrfs/inode.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index c7b75dd..b9310f8 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -4939,16 +4939,13 @@ static int maybe_insert_hole(struct btrfs_root *root, struct inode *inode,
>
> ret = btrfs_drop_extents(trans, root, inode, offset, offset + len, 1);
> if (ret) {
> - btrfs_abort_transaction(trans, ret);
> btrfs_end_transaction(trans);
> return ret;
> }
>
> ret = btrfs_insert_file_extent(trans, root, btrfs_ino(BTRFS_I(inode)),
> offset, 0, 0, len, 0, len, 0, 0, 0);
But here the extents have been already dropped and missing to insert the
items does not seem to lead to a consistent state.
It's possible that I'm missing something. In a call path that can be
safely rolled back even with a started transaction, we don't need to
abort in all cases. But if the rollback requires some non-trivial
modifications, I don't see options how to avoid the abort.
__btrfs_drop_extents does a lot of state changes and can itself fail
in the middle of dropping the range, aborting looks like the safest
option.
> - if (ret)
> - btrfs_abort_transaction(trans, ret);
> - else
> + if (!ret)
> btrfs_update_inode(trans, root, inode);
> btrfs_end_transaction(trans);
> return ret;
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Btrfs: do not abort transaction when failing to insert hole extent
2018-04-05 16:48 ` David Sterba
@ 2018-04-05 18:58 ` Liu Bo
2018-04-06 13:21 ` David Sterba
0 siblings, 1 reply; 8+ messages in thread
From: Liu Bo @ 2018-04-05 18:58 UTC (permalink / raw)
To: David Sterba, Liu Bo, linux-btrfs
On Thu, Apr 5, 2018 at 9:48 AM, David Sterba <dsterba@suse.cz> wrote:
> On Sat, Mar 31, 2018 at 06:11:55AM +0800, Liu Bo wrote:
>> This is running in a typical write path, not inside a critical path
>> where we have to abort the running transaction, so it's OK to return
>> errors to callers and eventually to userspace.
>
> I'm not sure this is entierly correct, several other places do not abort
> after btrfs_drop_extents as there's nothing that would leave the
> structres in some half-state.
>
>> Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
>> ---
>> fs/btrfs/inode.c | 5 +----
>> 1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>> index c7b75dd..b9310f8 100644
>> --- a/fs/btrfs/inode.c
>> +++ b/fs/btrfs/inode.c
>> @@ -4939,16 +4939,13 @@ static int maybe_insert_hole(struct btrfs_root *root, struct inode *inode,
>>
>> ret = btrfs_drop_extents(trans, root, inode, offset, offset + len, 1);
>> if (ret) {
>> - btrfs_abort_transaction(trans, ret);
>> btrfs_end_transaction(trans);
>> return ret;
>> }
>>
>> ret = btrfs_insert_file_extent(trans, root, btrfs_ino(BTRFS_I(inode)),
>> offset, 0, 0, len, 0, len, 0, 0, 0);
>
> But here the extents have been already dropped and missing to insert the
> items does not seem to lead to a consistent state.
>
> It's possible that I'm missing something. In a call path that can be
> safely rolled back even with a started transaction, we don't need to
> abort in all cases. But if the rollback requires some non-trivial
> modifications, I don't see options how to avoid the abort.
>
> __btrfs_drop_extents does a lot of state changes and can itself fail
> in the middle of dropping the range, aborting looks like the safest
> option.
>
As maybe_insert_hole is only called by btrfs_cont_expand here, which
means it's a really hole, I don't expect drop_extents would drop
anything, we can remove this drop_extents and put an assert after
btrfs_insert_file_extent for checking EEXIST.
It's different from punch hole where we need to explicitly drop an
actual extent and replace it with a hole range.
thanks,
liubo
>> - if (ret)
>> - btrfs_abort_transaction(trans, ret);
>> - else
>> + if (!ret)
>> btrfs_update_inode(trans, root, inode);
>> btrfs_end_transaction(trans);
>> return 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Btrfs: do not abort transaction when failing to insert hole extent
2018-04-05 18:58 ` Liu Bo
@ 2018-04-06 13:21 ` David Sterba
2018-04-06 17:43 ` Liu Bo
0 siblings, 1 reply; 8+ messages in thread
From: David Sterba @ 2018-04-06 13:21 UTC (permalink / raw)
To: Liu Bo; +Cc: David Sterba, Liu Bo, linux-btrfs
On Thu, Apr 05, 2018 at 11:58:16AM -0700, Liu Bo wrote:
> On Thu, Apr 5, 2018 at 9:48 AM, David Sterba <dsterba@suse.cz> wrote:
> > On Sat, Mar 31, 2018 at 06:11:55AM +0800, Liu Bo wrote:
> >> This is running in a typical write path, not inside a critical path
> >> where we have to abort the running transaction, so it's OK to return
> >> errors to callers and eventually to userspace.
> >
> > I'm not sure this is entierly correct, several other places do not abort
> > after btrfs_drop_extents as there's nothing that would leave the
> > structres in some half-state.
> >
> >> Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
> >> ---
> >> fs/btrfs/inode.c | 5 +----
> >> 1 file changed, 1 insertion(+), 4 deletions(-)
> >>
> >> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> >> index c7b75dd..b9310f8 100644
> >> --- a/fs/btrfs/inode.c
> >> +++ b/fs/btrfs/inode.c
> >> @@ -4939,16 +4939,13 @@ static int maybe_insert_hole(struct btrfs_root *root, struct inode *inode,
> >>
> >> ret = btrfs_drop_extents(trans, root, inode, offset, offset + len, 1);
> >> if (ret) {
> >> - btrfs_abort_transaction(trans, ret);
> >> btrfs_end_transaction(trans);
> >> return ret;
> >> }
> >>
> >> ret = btrfs_insert_file_extent(trans, root, btrfs_ino(BTRFS_I(inode)),
> >> offset, 0, 0, len, 0, len, 0, 0, 0);
> >
> > But here the extents have been already dropped and missing to insert the
> > items does not seem to lead to a consistent state.
> >
> > It's possible that I'm missing something. In a call path that can be
> > safely rolled back even with a started transaction, we don't need to
> > abort in all cases. But if the rollback requires some non-trivial
> > modifications, I don't see options how to avoid the abort.
> >
> > __btrfs_drop_extents does a lot of state changes and can itself fail
> > in the middle of dropping the range, aborting looks like the safest
> > option.
> >
>
> As maybe_insert_hole is only called by btrfs_cont_expand here, which
> means it's a really hole, I don't expect drop_extents would drop
> anything, we can remove this drop_extents and put an assert after
> btrfs_insert_file_extent for checking EEXIST.
Sounds good.
> It's different from punch hole where we need to explicitly drop an
> actual extent and replace it with a hole range.
Right, that's what I didn't see at first.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Btrfs: do not abort transaction when failing to insert hole extent
2018-04-06 13:21 ` David Sterba
@ 2018-04-06 17:43 ` Liu Bo
2018-04-10 1:23 ` Liu Bo
0 siblings, 1 reply; 8+ messages in thread
From: Liu Bo @ 2018-04-06 17:43 UTC (permalink / raw)
To: David Sterba, Liu Bo, Liu Bo, linux-btrfs
On Fri, Apr 6, 2018 at 6:21 AM, David Sterba <dsterba@suse.cz> wrote:
> On Thu, Apr 05, 2018 at 11:58:16AM -0700, Liu Bo wrote:
>> On Thu, Apr 5, 2018 at 9:48 AM, David Sterba <dsterba@suse.cz> wrote:
>> > On Sat, Mar 31, 2018 at 06:11:55AM +0800, Liu Bo wrote:
>> >> This is running in a typical write path, not inside a critical path
>> >> where we have to abort the running transaction, so it's OK to return
>> >> errors to callers and eventually to userspace.
>> >
>> > I'm not sure this is entierly correct, several other places do not abort
>> > after btrfs_drop_extents as there's nothing that would leave the
>> > structres in some half-state.
>> >
>> >> Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
>> >> ---
>> >> fs/btrfs/inode.c | 5 +----
>> >> 1 file changed, 1 insertion(+), 4 deletions(-)
>> >>
>> >> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>> >> index c7b75dd..b9310f8 100644
>> >> --- a/fs/btrfs/inode.c
>> >> +++ b/fs/btrfs/inode.c
>> >> @@ -4939,16 +4939,13 @@ static int maybe_insert_hole(struct btrfs_root *root, struct inode *inode,
>> >>
>> >> ret = btrfs_drop_extents(trans, root, inode, offset, offset + len, 1);
>> >> if (ret) {
>> >> - btrfs_abort_transaction(trans, ret);
>> >> btrfs_end_transaction(trans);
>> >> return ret;
>> >> }
>> >>
>> >> ret = btrfs_insert_file_extent(trans, root, btrfs_ino(BTRFS_I(inode)),
>> >> offset, 0, 0, len, 0, len, 0, 0, 0);
>> >
>> > But here the extents have been already dropped and missing to insert the
>> > items does not seem to lead to a consistent state.
>> >
>> > It's possible that I'm missing something. In a call path that can be
>> > safely rolled back even with a started transaction, we don't need to
>> > abort in all cases. But if the rollback requires some non-trivial
>> > modifications, I don't see options how to avoid the abort.
>> >
>> > __btrfs_drop_extents does a lot of state changes and can itself fail
>> > in the middle of dropping the range, aborting looks like the safest
>> > option.
>> >
>>
>> As maybe_insert_hole is only called by btrfs_cont_expand here, which
>> means it's a really hole, I don't expect drop_extents would drop
>> anything, we can remove this drop_extents and put an assert after
>> btrfs_insert_file_extent for checking EEXIST.
>
> Sounds good.
>
Let me make a v2 and have a fstests run.
thanks,
liubo
>> It's different from punch hole where we need to explicitly drop an
>> actual extent and replace it with a hole range.
>
> Right, that's what I didn't see at first.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Btrfs: do not abort transaction when failing to insert hole extent
2018-04-06 17:43 ` Liu Bo
@ 2018-04-10 1:23 ` Liu Bo
2018-04-10 12:12 ` David Sterba
0 siblings, 1 reply; 8+ messages in thread
From: Liu Bo @ 2018-04-10 1:23 UTC (permalink / raw)
To: David Sterba, Liu Bo, Liu Bo, linux-btrfs
On Fri, Apr 6, 2018 at 10:43 AM, Liu Bo <obuil.liubo@gmail.com> wrote:
> On Fri, Apr 6, 2018 at 6:21 AM, David Sterba <dsterba@suse.cz> wrote:
>> On Thu, Apr 05, 2018 at 11:58:16AM -0700, Liu Bo wrote:
>>> On Thu, Apr 5, 2018 at 9:48 AM, David Sterba <dsterba@suse.cz> wrote:
>>> > On Sat, Mar 31, 2018 at 06:11:55AM +0800, Liu Bo wrote:
>>> >> This is running in a typical write path, not inside a critical path
>>> >> where we have to abort the running transaction, so it's OK to return
>>> >> errors to callers and eventually to userspace.
>>> >
>>> > I'm not sure this is entierly correct, several other places do not abort
>>> > after btrfs_drop_extents as there's nothing that would leave the
>>> > structres in some half-state.
>>> >
>>> >> Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
>>> >> ---
>>> >> fs/btrfs/inode.c | 5 +----
>>> >> 1 file changed, 1 insertion(+), 4 deletions(-)
>>> >>
>>> >> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>>> >> index c7b75dd..b9310f8 100644
>>> >> --- a/fs/btrfs/inode.c
>>> >> +++ b/fs/btrfs/inode.c
>>> >> @@ -4939,16 +4939,13 @@ static int maybe_insert_hole(struct btrfs_root *root, struct inode *inode,
>>> >>
>>> >> ret = btrfs_drop_extents(trans, root, inode, offset, offset + len, 1);
>>> >> if (ret) {
>>> >> - btrfs_abort_transaction(trans, ret);
>>> >> btrfs_end_transaction(trans);
>>> >> return ret;
>>> >> }
>>> >>
>>> >> ret = btrfs_insert_file_extent(trans, root, btrfs_ino(BTRFS_I(inode)),
>>> >> offset, 0, 0, len, 0, len, 0, 0, 0);
>>> >
>>> > But here the extents have been already dropped and missing to insert the
>>> > items does not seem to lead to a consistent state.
>>> >
>>> > It's possible that I'm missing something. In a call path that can be
>>> > safely rolled back even with a started transaction, we don't need to
>>> > abort in all cases. But if the rollback requires some non-trivial
>>> > modifications, I don't see options how to avoid the abort.
>>> >
>>> > __btrfs_drop_extents does a lot of state changes and can itself fail
>>> > in the middle of dropping the range, aborting looks like the safest
>>> > option.
>>> >
>>>
>>> As maybe_insert_hole is only called by btrfs_cont_expand here, which
>>> means it's a really hole, I don't expect drop_extents would drop
>>> anything, we can remove this drop_extents and put an assert after
>>> btrfs_insert_file_extent for checking EEXIST.
>>
>> Sounds good.
>>
>
> Let me make a v2 and have a fstests run.
>
It turns out that the btrfs_drop_extents() here is quite necessary
since fallocate(2) has a FALLOC_FL_KEEP_SIZE option, and when that
happens, a hole extent would be appended between the EOF and fallocate
range's start, then a later truncate up would have to drop these hole
extents in order to expand with a new hole...
As I don't see a way to gracefully solve this except keeping
drop_extents(), lets drop this patch instead.
thanks,
liubo
> thanks,
> liubo
>
>>> It's different from punch hole where we need to explicitly drop an
>>> actual extent and replace it with a hole range.
>>
>> Right, that's what I didn't see at first.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Btrfs: do not abort transaction when failing to insert hole extent
2018-04-10 1:23 ` Liu Bo
@ 2018-04-10 12:12 ` David Sterba
2018-04-12 1:39 ` Liu Bo
0 siblings, 1 reply; 8+ messages in thread
From: David Sterba @ 2018-04-10 12:12 UTC (permalink / raw)
To: Liu Bo; +Cc: David Sterba, Liu Bo, linux-btrfs
On Mon, Apr 09, 2018 at 06:23:14PM -0700, Liu Bo wrote:
> >>> As maybe_insert_hole is only called by btrfs_cont_expand here, which
> >>> means it's a really hole, I don't expect drop_extents would drop
> >>> anything, we can remove this drop_extents and put an assert after
> >>> btrfs_insert_file_extent for checking EEXIST.
> >> Sounds good.
> > Let me make a v2 and have a fstests run.
> It turns out that the btrfs_drop_extents() here is quite necessary
> since fallocate(2) has a FALLOC_FL_KEEP_SIZE option, and when that
> happens, a hole extent would be appended between the EOF and fallocate
> range's start, then a later truncate up would have to drop these hole
> extents in order to expand with a new hole...
Would it make sense to split the cases where FALLOC_FL_KEEP_SIZE is and
is not used? Either passing an argument or 2 functions where one could
avoid the drop. I've looked at the code only briefly, so this may be a
nonsense in the end.
> As I don't see a way to gracefully solve this except keeping
> drop_extents(), lets drop this patch instead.
Understood.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Btrfs: do not abort transaction when failing to insert hole extent
2018-04-10 12:12 ` David Sterba
@ 2018-04-12 1:39 ` Liu Bo
0 siblings, 0 replies; 8+ messages in thread
From: Liu Bo @ 2018-04-12 1:39 UTC (permalink / raw)
To: David Sterba, Liu Bo, Liu Bo, linux-btrfs
On Tue, Apr 10, 2018 at 5:12 AM, David Sterba <dsterba@suse.cz> wrote:
> On Mon, Apr 09, 2018 at 06:23:14PM -0700, Liu Bo wrote:
>> >>> As maybe_insert_hole is only called by btrfs_cont_expand here, which
>> >>> means it's a really hole, I don't expect drop_extents would drop
>> >>> anything, we can remove this drop_extents and put an assert after
>> >>> btrfs_insert_file_extent for checking EEXIST.
>> >> Sounds good.
>> > Let me make a v2 and have a fstests run.
>> It turns out that the btrfs_drop_extents() here is quite necessary
>> since fallocate(2) has a FALLOC_FL_KEEP_SIZE option, and when that
>> happens, a hole extent would be appended between the EOF and fallocate
>> range's start, then a later truncate up would have to drop these hole
>> extents in order to expand with a new hole...
>
> Would it make sense to split the cases where FALLOC_FL_KEEP_SIZE is and
> is not used? Either passing an argument or 2 functions where one could
> avoid the drop. I've looked at the code only briefly, so this may be a
> nonsense in the end.
>
I'm afraid that It won't work as there is no way I could think of to
know whether an inode has been fallocate with KEEP_SIZE, IOW, seems
that we have to do a search to check if there are extra extents beyond
EOF.
thanks,
liubo
>> As I don't see a way to gracefully solve this except keeping
>> drop_extents(), lets drop this patch instead.
>
> Understood.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-04-12 1:39 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-30 22:11 [PATCH] Btrfs: do not abort transaction when failing to insert hole extent Liu Bo
2018-04-05 16:48 ` David Sterba
2018-04-05 18:58 ` Liu Bo
2018-04-06 13:21 ` David Sterba
2018-04-06 17:43 ` Liu Bo
2018-04-10 1:23 ` Liu Bo
2018-04-10 12:12 ` David Sterba
2018-04-12 1:39 ` Liu Bo
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.