Asynchronous deletes don't work

[dweeezil]

System information

Type	Version/Name
Distribution Name	Ubuntu
Distribution Version	16.04
Linux Kernel	4.2.8 and 4.16.2 stock upstream kernels
Architecture	x64_64
ZFS Version	0.7.0 and newer (see below), but tested with current master
SPL Version

Describe the problem you're observing

Deletes which should be asynchronous are not.

Describe how to reproduce the problem

Delete a file containing more blocks than the zfs_delete_blocks setting and observe control flow via either kernel or ZFS module instrumentation.

Include any warning/errors/backtraces from the system logs

N/A

Background

The zfs_delete_blocks, which was introduced via 6ca1fab in #4259 is supposed to allow for asynchronous deletes of large files in order that the various unlink system calls not block while the freeing takes place.

Cause of problem

The logic in zfs_remove() which determines whether a file can be removed "now" and whether it should be deleted asynchronously is not compatible with the handling of inode reference counts (i_count).

In the do_unlinkat() path, as used by a typical rm command, an extra reference is taken prior to calling vfs_unlink(). Then, in zfs_remove(), the zfs_zget() causes yet another reference to be taken. As a result, the minimum reference count on this path is 3 prior to examining the count to determine synchronicity.

In the typical NFS path, in which, the same zfs_zget() causes the reference count to typically be 2.

Neither path allow the logic in zfs_remove(), which depends on the reference being 1 (i.e. "allocated in the icache with no additional references), to work properly.

[dweeezil]

My initial thoughts for a fix is to refactor zfs_remove() into a pair of (or 3) functions, one of which would be used by the VFS and another of which would be used internally (i.e. for removing dir-based xattrs/directories). [EDIT: this scheme probably won't work]

I'll also note that it seems as if the zfs_delete_blocks feature may not work under illumos due to its similar VN_HOLD() in the zfs_dirent_lock()->zfs_zget() path.

[behlendorf]

@dweeezil I may be overlooking something from the description but it sounds as if the fix could be as straight forward and not checking the i_count at all. This was something we inherited from illumos's vp->v_count, but really this logic could be entirely based on size (and if it's mapped). Nothing will really be freed until the last inode reference is dropped.

[dweeezil]

@behlendorf You're correct, we can probably remove all of that handling from zfs_remove(). I've worked up a potential solution in fa20150 which works in my light local testing. This patch has left all the async stuff in zfs_remove() for now.

[stale]

This issue has been automatically marked as "stale" because it has not had any activity for a while. It will be closed in 90 days if no further activity occurs. Thank you for your contributions.