file: Add a non-blocking mode to foffset_lock()

[markj]

Summary

This is needed to avoid deadlocks when locking more than one file at a
time.

The implementation assumes that -1 is a valid sentinel value, which I
believe is the case.

[markj]

sys/kern/vfs_vnops.c

775

This is too simple: negative seek offsets are allowed for character devices.

[rmacklem]

sys/kern/vfs_vnops.c

775

For copy_file_range, only regular files are allowed,
so the -1 return should be ok for that case.

Not sure how you generalize it?
I suppose there could be an extra return argument
added?

[kib]

sys/kern/vfs_vnops.c

775

It would be a landmine to use the function like that, making -1 a special return value. Yes, IMO a return pointer arg to return the calculated offset, and reserving the return value to errno, is the most natural extension. Mark is more than capable to design this.

[rmacklem]

sys/kern/vfs_vnops.c

775

Of course, so long as foffset_lock() isn't
considered a stable KAPI that cannot
be changed by an MFC.

I wasn't sure if that was the case?

[kib]

sys/kern/vfs_vnops.c

775

It is part of the core interfaces that should not be touched by any consumer that depends on the stability of KPI/KBI. In other words, I consider the change as fine even for stable branches.

Quick search in the source tree confirms my opinion: foffset_ functions are used by vfs/shmfd/devfs. shmfd/devfs try to micro-optimize the io path and provide file methods instead of going through VFS, which explains the need for foffset_ use.

[markj]

Add foffset_trylock(), which can signal an error out-of-band in
addition to returning an offset.

[kib]

sys/kern/vfs_vnops.c

846–847

[markj]

sys/kern/vfs_vnops.c

775

I wrote the update before reading these follow-up comments, but I avoided changing the existing interface and instead added a foffset_trylock(). I don't really see how foffset_* is a core interface: any module can register its own file type and methods, so it's reasonable IMO for them to make use of offset locking as well...

[markj]

foffset_lock1() has local scope.

[kib]

sys/kern/vfs_vnops.c

775

The fact that modules can use the interface does not mean that we (should) consider it stable. Same as we e.g. do not support other things like struct thread allocation by modules, or lot of other ways to manipulate kernel.
IMO the stable part of KBI is around drivers and might be file systems.

[markj]

sys/kern/vfs_vnops.c

775

My working assumption is that there is no hard boundary between stable or not. We should only try to preserve KBIs which may reasonably have out-of-tree consumers, and only when doing so is not too stressful.

IMO it is much more likely that some 3rd party code registers its own file type and locks file offsets than tries to allocate its own threads.

BTW, the main reason I do not really want to change the foffset_lock() interface is that only the trylock mode can result in an error. That is, the interface would become more complex for all consumers, even though most of them don't need to handle errors. From an interface perspective, I think it's cleaner to have a separate function for trylocks.

[rmacklem]

Looks fine to me, rick