Add fspacectl(2), vn_deallocate(9) and VOP_DEALLOCATE(9).

[khng]

Summary

Add fspacectl(2), vn_deallocate(9) and VOP_DEALLOCATE(9).

fspacectl(2) is a system call to provide space management support to
userspace applications. VOP_DEALLOCATE(9) is a VOP call to perform the
deallocation. vn_deallocate(9) is a public KPI.

The purpose of proposing a new system call, a KPI and a VOP call is to
allow bhyve or other hypervisor monitors to emulate the behavior of SCSI
UNMAP/NVMe DEALLOCATE on a plain file.

fspacectl(2) comprises of cmd and flags parameters to specify the
space management operation to be performed. Currently cmd can be
SPACECTL_DEALLOC, and flags has to be 0 for SPACECTL_DEALLOC
operation.

fo_fspacectl is added to fileops.
VOP_DEALLOCATE(9) is added as a new VOP call. A trivial implementation
of VOP_DEALLOCATE(9) is provided.

Submitted by: Ka Ho Ng <[e-mail]>
Sponsored by: The FreeBSD Foundation

[brooks]

sys/compat/freebsd32/freebsd32_misc.c

3573

With the removal of spacectl_range32 this whole function is redundant.

[khng]

• Move CTASSERT(sizeof(struct spacectl_range) == 16) from freebsd32_misc.c to sys_generic.c
• Remove freebsd32_fspacectl()

[khng]

sys/kern/vnode_if.src

795

This is for MNT_SHARED_WRITES(mp) == true, which is the case in ZFS.

[khng]

Updates:

• vn_deallocate_impl() does bwillwrite() and vn_start_write() on every iteration instead of before and after all the iterations.
• vn_deallocate() no longer takes a 'struct thread *'.

[khng]

_PC_FDEALLOC_PRESENT -> _PC_DEALLOC_PRESENT

[delphij]

lib/libc/sys/Symbol.map

413

Since we are now at FreeBSD 14, this should be in FBSD_1.7 namespace.

(Unrelated: I always wonder why we can't just call future versions e.g. FBSD_15.0; I usually find myself looking at Version.def to confirm)

sys/kern/sys_generic.c

110

I think it's worth commenting here that the reason we are CTASSERT'ing the size was because we wanted to make sure that for both 32-bit and 64-bit variants, the struct have the same layout (and thus the compat32 call can just use the same struct); having it in the same block of asserting size_t was confusing for me in the first glance.

918

Is this intentional? If so, could you please add a comment explaining why we are ignoring ERESTART/EINTR/EWOULDBLOCK? (I think this is different from short read/write and these shouldn't be ignored?)

[khng]

Address delphij@'s comments.

[khng]

sys/kern/sys_generic.c

918

It was intentional, but yes they shouldn't be ignored as short read/write.

[khng]

Manpage updates:

• Author part fixes
• Remove the unnecessary partial success description from fspacectl.2. The value of 0 is for complete successful operation only.

[kib]

sys/kern/sys_generic.c

918

I believe that was already discussed, and now you returned back the same errant behavior. Unix convention is that error return is only acceptable if the file content was not changed at all. If syscall did modified the file, regardless was it completed WRT requested op, or just a partial execution, userspace must know exactly what was done.

[khng]

• Move rangelock around iteration loop to make it clearer
• fspacectl(2) returns 0 as long as the file is modified, to prevent the system call being restarted after signals.

[kib]

sys/compat/freebsd32/syscalls.master

1177

I think that this requires translation for compat32, because spacectl_range contains off_t. Look how freebsd32_lseek() handles that, in particular the big- vs. little-endian versions of PAIR32TO64.

sys/kern/sys_generic.c

876

I wonder if it make sense to change the fspacectl(2) interface while not too late and split in and out spacectl_range. Basically keep original spacectl_range as is, and copy out optional processed length:

fspacectl(int fd, int cmd, const struct spacectl_range *sr, int flags, off_t *length);

902

Why don't you use fget_write()? I am not even sure why you check for CAP_PWRITE instead of CAP_WRITE, the later seems to be more suitable.

sys/kern/uipc_shm.c

1902 ↗

(On Diff #93055)

This was copied from shm_dotruncate_locked(), right? It misses the vm_pager_has_page() case. Basically, the page that you need to partially zero might be not resident but swap contains the user data. So we need to read the page from swap.

Perhaps you should refactor code to instantiate the page from shm_dotruncate_locked() and reuse it. Perhaps the code should take some control which region of the page should be cleared, to be passed to pmap_zero_page_area().

[khng]

sys/kern/sys_generic.c

876

Does it sound better to have copyout spacectl_range instead of copyout length, so that callers may save an fseek(DATA) call to guess where to restart?

902

But if we can in theory operate in any positions in a file, we should use CAP_PWRITE?

[khng]

• Updated fspacectl(2) to contain an optional parameter to store unprocessed operation range after the syscall returns
• Updated fspacectl.2 because of ^^^
• Added fspacectl(2) compat32 translation
• Remove uipc_shm (This should end up as a separate review)
• fget_write(CAP_PWRITE) instead of fget(CAP_PWRITE) in kern_fspacectl

[khng]

Restore line 1116 in sys/kern/vfs_default.c

[kib]

sys/kern/sys_generic.c

876

Why do you copyout result only in case of an error? This is esp. not useful for ERESTART/EINTR etc case where you cleared the error.

sys/kern/vfs_vnops.c

2350–2351

I suggest to extract introduction of vn_bmap_seekhole_locked() into a separate commit and do it in advance.

2403

This assert should be either repeated in vn_bmap_seekhole_locked(), or moved to that function.

3364

Use bool?

3373

if ((ioflg & (IO_NODELOCKED | IO_RANGELOCKED)) == 0)

[khng]

• Fix sys_fspacectl(2) copying out rmsr in opposite cases.
• Separate vn_bmap_seekhole_locked to a separate differential
• styles and use bool in vn_deallocate_impl

[khng]

Fix mangled git commit order.

[kib]

sys/kern/capabilities.conf

232 ↗

(On Diff #93206)

s/descriptors/operations/, perhaps?

[kib]

sys/compat/freebsd32/freebsd32_misc.c

3876

This condition is still not fixed.

BTW, I suggest to remove the error != 0 check at all. Copy *rqsr into *rmsr at the very beginning of kern_fspacectl() if rmsr != NULL, and it should be fine.

[khng]

Rebased on top of D31405.

[khng]

• Fix compat32's fspacectl error cases
• kern_fspacectl asserts rqsr be non-NULL

[khng]

kern_fspacectl should not KASSERT because of rqsr, instead it should return EINVAL to align with other EINVAL cases.

[khng]

Minor change in kern_fspacectl(2): No need to copy rqsr into *rmsrp in the very beginning since things in *rmsrp is useless if error != 0. This reduces the lines further.

[kib]

sys/kern/capabilities.conf

232 ↗

(On Diff #93206)

You did not fixed this

sys/kern/sys_generic.c

924

I still insist on copying out rmsr always, not only on error == 0. For instance, if errno == ENOSPC, you still might have modified parts of the file, and it is important for usermode to know.

[khng]

• fspacectl(2) now also returns rmsr in case return value is not 0.
• ^^^ Changes in fspacectl.2

[khng]

lib/libc/sys/fspacectl.2

152

[khng]

lib/libc/sys/fspacectl.2

154

[kib]

sys/kern/sys_generic.c

878

No, I think error is more important than cerror.

In other words, it should be

if (error == 0)
     error = cerror;

[khng]

• Returns error instead of copyout(9) error if error != 0.
• fspacectl.2 minor fixes.