p9fs: add socket transport

[njain15_protonmail.com]

Summary

Add a default socket transport to p9fs.

[markj]

sys/dev/virtio/p9fs/virtio_p9fs.c

97

In general we shouldn't really rename sysctls. That would break scripts, sysctl.conf configurations, etc..

I think this timeout is kind of bogus anyway, I suspect it's a workaround for some hangs which I fixed in commit 1f6f247b3526abd4d5927684735012220aefe373.

sys/fs/p9fs/trans/inet_p9fs.c

404 ↗

(On Diff #183353)

We should validate this more carefully, the user should get an error if they specify port 424242 or whatever.

426 ↗

(On Diff #183353)

What's the purpose of having a dedicated thread? 9p is a synchronous protocol. Each thread which submits a request can block directly on the socket rather than going through the indirection of an extra thread.

536 ↗

(On Diff #183353)

Rather than having a timeout, I suspect it'd be better to specify PCATCH here, so that a blocked thread can be interrupted by a signal. You'll get that for free if you let request threads block in soreceive().

552 ↗

(On Diff #183353)

I don't really agree with this naming: there's nothing here that's inherent to INET sockets (except for a couple of TCP options which can be made conditional on the protocol family). This transport could work just as well with unix sockets. Can we call this a "socket" transport instead?

sys/modules/p9fs/Makefile

9

Is there any real reason to have a separate directory for this? Do you plan to add more transports? If not I'd suggest just calling this file p9_transport_socket.c and keeping it in the main directory.

sys/modules/p9fs/trans/Makefile

6 ↗

(On Diff #183353)

I don't think there's any real reason to have a separate kernel module here. I understand that the virtio transport has one, but here you are just using the core kernel's interfaces, and p9fs is already a separate module. It is okay to just have this transport be built in to the p9fs module.

[njain15_protonmail.com]

sys/dev/virtio/p9fs/virtio_p9fs.c

97

Yes, I agree. Sorry about that.

sys/fs/p9fs/trans/inet_p9fs.c

404 ↗

(On Diff #183353)

Implemented.

426 ↗

(On Diff #183353)

It may be synchronous, but is also concurrent? If each thread blocks on the socket, then I considered these issues:

• Even with a PCATCH, if a bad server decided to drop response for a request, it would never exit sleep unless interrupted (hence the timeout).
• Having multiple threads sbwait() on the same lock will cause all of them to wake up at once (since they each have to check whether the tag is theirs). I just don't think a broadcasted wakeup is the right approach here (unless this is not how sbwait() works).
• Having a separate thread was cleaner for both the design and implementation to me (especially for error handling, like disconnecting the socket). Having control over queuing requests allows, for example, in the future to attempt to create a new socket for this channel in case of an error easily.

I don't know though. I would have to run a performance measure against the socket receive buffer locking one.

536 ↗

(On Diff #183353)

Modified to be either timeout or interrupt.

552 ↗

(On Diff #183353)

Changed.

sys/modules/p9fs/Makefile

9

I was planning on splitting up virtio_p9fs into just the device driver and the transport. If that doesn't sound like a good idea, then I will make the suggested change.

sys/modules/p9fs/trans/Makefile

6 ↗

(On Diff #183353)

Agreed. I can have a SYSINIT like you did, or just add the socket transport by default in p9fs_init(). What sounds more reasonable?

[markj]

sys/dev/virtio/p9fs/virtio_p9fs.c

97

There is no need to apologize.

sys/fs/p9fs/trans/inet_p9fs.c

191 ↗

(On Diff #183602)

Is it possible to have error == 0 here? I think not.

233 ↗

(On Diff #183602)

Why do you use the lowat threshold here? Why not read as soon as there are sizeof(hdr) bytes available?

Ok, I see that you set SO_RCVLOWAT, but I'm not sure why. You are only reading from the socket once that many bytes are already available, so it doesn't matter. RCVLOWAT is basically "the number of bytes that need to be present in the receive socket buffer before soreceive() will return them." I believe the default is 1.

241 ↗

(On Diff #183602)

So, in FreeBSD, explicit CVs are basically never needed. You can sleep directly on the lock using mtx_sleep(). You can keep it here if you prefer but I don't think it's really needed.

267 ↗

(On Diff #183602)

Better to explicitly state what those reasons are.

405 ↗

(On Diff #183602)

Note, strtol() returns a long, not an int.

406 ↗

(On Diff #183602)

0 is also an invalid port number, but I guess soconnect() will catch that.

426 ↗

(On Diff #183353)

Even with a PCATCH, if a bad server decided to drop response for a request, it would never exit sleep unless interrupted (hence the timeout).

If you want a timeout, note that you can set the SO_RCVTIMEO socket option for this purpose.

Having multiple threads sbwait() on the same lock will cause all of them to wake up at once

Yes, that's fair. Any time data arrives on the socket, all the threads will be awoken.

I think you could work around this by having threads block on the individual request structure, rather than using sbwait(). The socket upcall could figure out which pending request is receiving a response, and then wake up only that thread.

Having a separate thread was cleaner for both the design and implementation to me (especially for error handling, like disconnecting the socket).

Ok. I'm fine with keeping it that way for now.

I don't know though. I would have to run a performance measure against the socket receive buffer locking one.

My gut says that it won't matter very much in practice. Any time you have N threads trying to interact with a shared queue, the queue will be a bottleneck once N is big enough. 9pfs is fundamentally not going to scale well to many threads since you only get one channel to communicate with the server; the virtio transport has the same problem. Moving some work into a dedicated thread might help push the bottleneck down, but it's still there.

Note that indirection through a worker thread also imposes a latency penalty: whenever data is received on the socket, the upcall wakes up the worker thread, which reads data from the socket buffer and then wakes up the original requestor. That indirection is not free; going to sleep and waking up involves making trips through the scheduler. I'm not saying it's the wrong tradeoff here, it's just something to be aware of.

sys/modules/p9fs/Makefile

9

To be honest this sounds like busy-work to me. Is there some specific problem that would addressed by splitting up the modules this way? Are you planning to add some additional transports? If not, I'd just keep things simple.

sys/modules/p9fs/trans/Makefile

6 ↗

(On Diff #183353)

I think adding it directly from p9fs_init() is fine.

[njain15_protonmail.com]

Refactorted with following changes:

• Migrate from msleep to mtx_sleep. Remove cond vars.
• Remove module and bake the transport within p9fs.

---

Is it possible to have error == 0 here? I think not.

Removed.

Ok, I see that you set SO_RCVLOWAT, but I'm not sure why. You are only reading from the socket once that many bytes are already available, so it doesn't matter. RCVLOWAT is basically "the number of bytes that need to be present in the receive socket buffer before soreceive() will return them." I believe the default is 1.

soreadable() in the upcall checks if the number of available bytes is greater than the receive low watermark (which in this case is the header size), and then wakes up the thread. Then, when the thread loops, we can just check against RCVLOWAT instead of sbavail >= sizeof(hdr).

So, in FreeBSD, explicit CVs are basically never needed. You can sleep directly on the lock using mtx_sleep(). You can keep it here if you prefer but I don't think it's really needed.

Removed. The thread sleeps on &chan->wake now. In the case of in9p_close(), it wakes up the handler on &chan->td_running.

Better to explicitly state what those reasons are.

Added.

I think you could work around this by having threads block on the individual request structure, rather than using sbwait(). The socket upcall could figure out which pending request is receiving a response, and then wake up only that thread.

My reason for not doing that essentially boils to moving away from coarse-grained locking. I thought using one lock for everything might cause high contention later on. I will look more into this in the future - maybe even looking into lockless MP-SC queues. Although, I think at that point, it's probably not worth it - the P9 protocol is inherently slow.

To be honest this sounds like busy-work to me. Is there some specific problem that would addressed by splitting up the modules this way? Are you planning to add some additional transports? If not, I'd just keep things simple.

Moved the transport to p9fs/ directory.

[markj]

sys/fs/p9fs/p9_trans_socket.c

28

Extra line here.

80

This file is still using the in9p_ prefix everywhere, it should be socket9p or sock9p or so.

99

This list doesn't appear to actually get used. Do you have some plan for it?

240

PCATCH means, "wake me up if a signal is delivered to this thread while I'm asleep," but this is a kernel thread that doesn't receive signals.

359

You shouldn't manipulate socket state directly like this. Why do you need to clear this flag? The loop should look like kern_connectat().

(At some point we should factor this code out into a helper function. There are several other soconnect() callers which have nearly identical loops.)

365

Why clear the error? If there's a connection error shouldn't we clean everything up and free the socket?

475

I guess it's subjective, but this kind of comment is useless IMO.

[njain15_protonmail.com]

sys/fs/p9fs/p9_trans_socket.c

99

Right, it was only there for module load/unload purposes.

359

I read it in the socket(9) man page.

If soconnect() fails, the caller must manually clear the SS_ISCONNECTING flag.

365

error is checked below, and will eventually lead to a soclose and cleanup.

[markj]

sys/fs/p9fs/p9_trans_socket.c

99

Can we get rid of it then?

359

Oof, ok. I am not sure if that's really correct, but now that you say it, I do see that pattern elsewhere, e.g., in clnt_vc_create(). It's probably ok to leave it for now then.

365

Are you sure? My reading of p9_client_create() is that if the transport's create() method fails, we'll just free the client structure and return without doing any cleanup. Where does the soclose() come from?

[njain15_protonmail.com]

sys/fs/p9fs/p9_trans_socket.c

365

Hold on, let me check. My inline comments were sent before the updated diff for some reason.

[njain15_protonmail.com]

sys/fs/p9fs/p9_trans_socket.c

365

sock9p_create() calls sock9p_sock_create(). If it fails, it goes to (err). There, if the socket is not null it is closed and the channel is freed.

[markj]

Mostly looking good!

sys/fs/p9fs/p9_trans_socket.c

365

Ok, I think I assumed that sock9p_sock_create() was the transport's create() implementation, sorry for the noise.

(But I still don't see why we should set so->so_error = 0.)

383

It'd be nice to deal with this TODO.

425

Extra newline here.

526

Ouch, so anyone can break the channel by sending ctrl-C while a process is blocked here? I don't think that's really workable.

If I'm reading correctly, we can defer reuse of the tag by setting req->tc.tag = P9_NOTAG. See p9_free_req(). Can we handle signals by allowing the requesting thread to return, but clearing the tag and letting the transport layer free it later once we get a reply?

542

I don't think this can be const, in fact, since p9_register_trans() will modify it when inserting into the transport list.