Btrfs devel - Jan 2013 - Deleted subvolume reappears and other cleaner issues

Hi,
I want to check if any of the below issues are worth/should be  fixed:

# btrfs_ioctl_snap_destroy() does not commit a transaction. As a
result, user can ask to delete a subvol, he receives "ok" back. Even
if user does "btrfs sub list",
he will not see the deleted subvol (even though the transaction was
not committed yet). But if a crash happens, ORPHAN_ITEM will not
re-appear after crash.
So after crash, the subvolume still exists perfectly fine (happened
couple of times here).

# btrfs_drop_snapshot() does not commit a transaction after
btrfs_del_orphan_item(). So if the subvol deletion completed in one go
(did not have to detach and re-attach to transaction, thus committing
the ORPHAN_ITEM and drop_progress/level), then after crash ORPHAN_ITEM
will not be in the tree, and subvolume still exists.

# btrfs_drop_snapshot() checks btrfs_should_end_transaction(), and
then does btrfs_end_transaction_throttle() and
btrfs_start_transaction(). However, it looks like it can rejoin the
same transaction if transaction was not not blocked yet. Minor issue,
perhaps?

# umount may get delayed because of pending-for-deletion subvolumes:
btrfs_commit_super() locks the cleaner_mutex, so it will wait for the
cleaner to complete.
On the other hand, cleaner will not give up until it completes
processing all its splice. If currently cleaner is not running, then
btrfs_commit_super()
calls btrfs_clean_old_snapshots() directly. So does it make sense:
- btrfs_commit_super() will not call btrfs_clean_old_snapshots()
- close_ctree() calls kthread_stop(cleaner_kthread) early, and cleaner
thread periodically checks if it needs to exit

Thanks,
Alex.
--
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

Can anyone please comment on below?

On Thu, Jan 31, 2013 at 3:03 PM, Alex Lyakas
<alex.btrfs@zadarastorage.com> wrote:
> Hi,
> I want to check if any of the below issues are worth/should be  fixed:
>
> # btrfs_ioctl_snap_destroy() does not commit a transaction. As a
> result, user can ask to delete a subvol, he receives "ok" back.
Even
> if user does "btrfs sub list",
> he will not see the deleted subvol (even though the transaction was
> not committed yet). But if a crash happens, ORPHAN_ITEM will not
> re-appear after crash.
> So after crash, the subvolume still exists perfectly fine (happened
> couple of times here).
>
> # btrfs_drop_snapshot() does not commit a transaction after
> btrfs_del_orphan_item(). So if the subvol deletion completed in one go
> (did not have to detach and re-attach to transaction, thus committing
> the ORPHAN_ITEM and drop_progress/level), then after crash ORPHAN_ITEM
> will not be in the tree, and subvolume still exists.
>
> # btrfs_drop_snapshot() checks btrfs_should_end_transaction(), and
> then does btrfs_end_transaction_throttle() and
> btrfs_start_transaction(). However, it looks like it can rejoin the
> same transaction if transaction was not not blocked yet. Minor issue,
> perhaps?
>
> # umount may get delayed because of pending-for-deletion subvolumes:
> btrfs_commit_super() locks the cleaner_mutex, so it will wait for the
> cleaner to complete.
> On the other hand, cleaner will not give up until it completes
> processing all its splice. If currently cleaner is not running, then
> btrfs_commit_super()
> calls btrfs_clean_old_snapshots() directly. So does it make sense:
> - btrfs_commit_super() will not call btrfs_clean_old_snapshots()
> - close_ctree() calls kthread_stop(cleaner_kthread) early, and cleaner
> thread periodically checks if it needs to exit
>
> Thanks,
> Alex.
--
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 Thu, Jan 31, 2013 at 06:03:06AM -0700, Alex Lyakas
wrote:
> Hi,
> I want to check if any of the below issues are worth/should be  fixed:
> 
> # btrfs_ioctl_snap_destroy() does not commit a transaction. As a
> result, user can ask to delete a subvol, he receives "ok" back.
Even
> if user does "btrfs sub list",
> he will not see the deleted subvol (even though the transaction was
> not committed yet). But if a crash happens, ORPHAN_ITEM will not
> re-appear after crash.
> So after crash, the subvolume still exists perfectly fine (happened
> couple of times here).
Same thing happens to normal unlinks, I don''t see a reason to have
different
rules for subvols.
> 
> # btrfs_drop_snapshot() does not commit a transaction after
> btrfs_del_orphan_item(). So if the subvol deletion completed in one go
> (did not have to detach and re-attach to transaction, thus committing
> the ORPHAN_ITEM and drop_progress/level), then after crash ORPHAN_ITEM
> will not be in the tree, and subvolume still exists.
> 
Again same thing happens with normal files.
> # btrfs_drop_snapshot() checks btrfs_should_end_transaction(), and
> then does btrfs_end_transaction_throttle() and
> btrfs_start_transaction(). However, it looks like it can rejoin the
> same transaction if transaction was not not blocked yet. Minor issue,
> perhaps?
No if we didn''t block then its ok and we wait longer, we only throttle
to give
the transaction stuff a chance to commit, so if the join logic decides its ok to
go on then we''re good.
> 
> # umount may get delayed because of pending-for-deletion subvolumes:
> btrfs_commit_super() locks the cleaner_mutex, so it will wait for the
> cleaner to complete.
> On the other hand, cleaner will not give up until it completes
> processing all its splice. If currently cleaner is not running, then
> btrfs_commit_super()
> calls btrfs_clean_old_snapshots() directly. So does it make sense:
> - btrfs_commit_super() will not call btrfs_clean_old_snapshots()
> - close_ctree() calls kthread_stop(cleaner_kthread) early, and cleaner
> thread periodically checks if it needs to exit
I don''t quite follow this, but sure?  Thanks,

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

Thanks for your comments, Josef.

Another thing that confuses me is that there are some cases, in which
btrfs_drop_snapshot() has a failure, but still returns 0, like for
example, if btrfs_del_root() fails. (For cases when
btrfs_drop_snapshot() returns non-zero there is a BUG_ON).
So in this case for me __btrfs_abort_transaction() sees that
trans->blocks_used==0, so it doesn''t call __btrfs_std_error, which
would further force the filesystem to become RO. So after that
btrfs_drop_snapshot successfully completes, and, basically, nobody
will retry the subvol deletion. In addition, in this case, after
couple of seconds the machine completely freezes for me. I have not
yet succeeded to determine why.

Thanks,
Alex.


On Wed, Feb 6, 2013 at 5:14 PM, Josef Bacik <jbacik@fusionio.com>
wrote:
> On Thu, Jan 31, 2013 at 06:03:06AM -0700, Alex Lyakas wrote:
>> Hi,
>> I want to check if any of the below issues are worth/should be  fixed:
>>
>> # btrfs_ioctl_snap_destroy() does not commit a transaction. As a
>> result, user can ask to delete a subvol, he receives "ok"
back. Even
>> if user does "btrfs sub list",
>> he will not see the deleted subvol (even though the transaction was
>> not committed yet). But if a crash happens, ORPHAN_ITEM will not
>> re-appear after crash.
>> So after crash, the subvolume still exists perfectly fine (happened
>> couple of times here).
>
> Same thing happens to normal unlinks, I don''t see a reason to have
different
> rules for subvols.
>
>>
>> # btrfs_drop_snapshot() does not commit a transaction after
>> btrfs_del_orphan_item(). So if the subvol deletion completed in one go
>> (did not have to detach and re-attach to transaction, thus committing
>> the ORPHAN_ITEM and drop_progress/level), then after crash ORPHAN_ITEM
>> will not be in the tree, and subvolume still exists.
>>
>
> Again same thing happens with normal files.
>
>> # btrfs_drop_snapshot() checks btrfs_should_end_transaction(), and
>> then does btrfs_end_transaction_throttle() and
>> btrfs_start_transaction(). However, it looks like it can rejoin the
>> same transaction if transaction was not not blocked yet. Minor issue,
>> perhaps?
>
> No if we didn''t block then its ok and we wait longer, we only
throttle to give
> the transaction stuff a chance to commit, so if the join logic decides its
ok to
> go on then we''re good.
>
>>
>> # umount may get delayed because of pending-for-deletion subvolumes:
>> btrfs_commit_super() locks the cleaner_mutex, so it will wait for the
>> cleaner to complete.
>> On the other hand, cleaner will not give up until it completes
>> processing all its splice. If currently cleaner is not running, then
>> btrfs_commit_super()
>> calls btrfs_clean_old_snapshots() directly. So does it make sense:
>> - btrfs_commit_super() will not call btrfs_clean_old_snapshots()
>> - close_ctree() calls kthread_stop(cleaner_kthread) early, and cleaner
>> thread periodically checks if it needs to exit
>
> I don''t quite follow this, but sure?  Thanks,
>
> Josef
--
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 Thu, Jan 31, 2013 at 03:03:06PM +0200, Alex Lyakas
wrote:
> # umount may get delayed because of pending-for-deletion subvolumes:
> btrfs_commit_super() locks the cleaner_mutex, so it will wait for the
> cleaner to complete.
> On the other hand, cleaner will not give up until it completes
> processing all its splice. If currently cleaner is not running, then
> btrfs_commit_super()
> calls btrfs_clean_old_snapshots() directly. So does it make sense:
> - btrfs_commit_super() will not call btrfs_clean_old_snapshots()
> - close_ctree() calls kthread_stop(cleaner_kthread) early, and cleaner
> thread periodically checks if it needs to exit
This is on my list of annoyances for a long time and I have wip patches
to fix that. I''ll send what I have for review.

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