vn_start_write(): consistently set *mpp to NULL on error or after failed sleep

[kib]

Summary

This ensures that *mpp != NULL iff vn_finished_write() should be called,
regardless of the returned error.  The only exception that must be
maintained is the case where vn_start_write(V_NOWAIT) is called with the
intent of later dropping other locks and then doing
vn_start_write(V_XSLEEP), which needs the mp value calculated from the
non-waitable call above it.  Add V_FORXSLEEP flag, which could be
combined with V_NOWAIT, and use it in such situations.

Also note that V_XSLEEP is not supported by vn_start_secondary_write().

[mjg]

i have a forgotten branch to sort out this api, i'll refresh it

in the meantime this is probably good

[rmacklem]

LGTM

[kib]

The previous version was too simple-minded, it breaks the common pattern of vn_start_write(&mp, V_NOWAIT) and then vn_start_write(&mp, V_XSLEEP). V_NOWAIT call in such cases must return mp.

Currently being tested by Peter.

[markj]

I don't understand what problem this is solving. Why is it so important to avoid setting *mpp in the failure case that we need to add a new flag?

sys/kern/vfs_vnops.c

1939

This could still be return (vn_start_write_refed(mpp, flags, false));?

[rmacklem]

In D39441#898170, @markj wrote:

I don't understand what problem this is solving. Why is it so important to avoid setting *mpp in the failure case that we need to add a new flag?

I'm kinda the guilty party here. In D39419 pjd@ noted that I
checked mp != NULL to decide to call vn_finished_write().
I countered that when the vp argument != NULL, that this
was sufficient (you can actually call it unconditionally, since
it checks for mp == NULL and just returns).
This inspired kib@ to make vn_start_write() always set mp non-NULL
if vn_finished_write() needs to be called and mp == NULL when it does
not need to be called.
--> However, it has turned out that this cannot be done for the V_XSLEEP

case with the vp argument == NULL.

To be honest, if I was kib@, I'd just abandon the exercise, but that is
obviously up to him.

[kib]

Remove unneeded chunk

[kib]

In D39441#898177, @rmacklem wrote:

In D39441#898170, @markj wrote:

I don't understand what problem this is solving. Why is it so important to avoid setting *mpp in the failure case that we need to add a new flag?

I'm kinda the guilty party here. In D39419 pjd@ noted that I
checked mp != NULL to decide to call vn_finished_write().
I countered that when the vp argument != NULL, that this
was sufficient (you can actually call it unconditionally, since
it checks for mp == NULL and just returns).
This inspired kib@ to make vn_start_write() always set mp non-NULL
if vn_finished_write() needs to be called and mp == NULL when it does
not need to be called.
--> However, it has turned out that this cannot be done for the V_XSLEEP

case with the vp argument == NULL.

To be honest, if I was kib@, I'd just abandon the exercise, but that is
obviously up to him.

The V_XSLEEP is a very special thing, I do not believe that it is needed anywhere outside the very core VFS, and even there it could be done differently (but not in this patch).
The goal is to reduce confusion for VFS consumers, in particular, by maintaining the invariant that *mpp != NULL after vn_start_write() implies the need to call vn_finish_write().
IMO it is worth a single flag.

[markj]

Doesn't the call in kern_sync() also need adjustment? If vn_start_write() fails, the rest of the loop body expects mp != NULL. Same in linux_syncfs(), I believe.

The call in vm_pageout_clean() no longer needs to set mp = NULL after vn_start_write() fails.

The V_XSLEEP is a very special thing, I do not believe that it is needed anywhere outside the very core VFS, and even there it could be done differently (but not in this patch).
The goal is to reduce confusion for VFS consumers, in particular, by maintaining the invariant that *mpp != NULL after vn_start_write() implies the need to call vn_finish_write().

vn_start_write(V_NOWAIT) is also not used much outside of the core VFS. IMHO it would be a bit simpler and more helpful to preserve *mpp unconditionally when that flag is set, instead of requiring a new one.

[kib]

Only zero *mpp for !V_NOWAIT calls.