On Fri, Oct 20, 2023 at 10:51:46PM +0200, Alexander Bluhm wrote:
> On Mon, Oct 16, 2023 at 10:17:50PM +0300, Vitaliy Makkoveev wrote:
> > This socket comes from userland, so this flag is never set. This makes
> > SB_NOINTR flag immutable, because we only set this bit on NFS client
> > socket buffers for all it's lifetime.
> >
> > I want to do this because this flag modifies sblock() behaviour and it's
> > not clean which lock should protect it for standalone sblock().
> >
> > ok?
>
> This code came here:
> https://svnweb.freebsd.org/csrg/sys/nfs/nfs_syscalls.c?revision=41903&view=markup
> "update from Rick Macklem adding TCP support to NFS"
>
> I would guess that this flag must be cleared to allow to kill the
> NFS server if a client does not respond. Otherwise it may hang in
> sbwait() in soreceive() or sosend().
>
> As the flags are never set, your diff does not change behavior.
>
> > I want to completely remove SB_NOINTR flag. Only NFS client sets it, but
> > since the socket never passed to userland, this flag is useless, because
> > we can't send singnal to kernel thread. So for this sblock()/sbwait()
> > sleep is uninterruptible. But I want to not mix NFS server and NFS
> > client diffs.
>
> The NFS client does not run as kernel thread, but as the process
> that does file system access. You can Ctrl-C it if mount uses
> "intr" option. NFSMNT_INT sets some PCATCH and sb_timeo_nsecs, but
> I think signal should also abort sbwait(). That is why NFS client
> sets SB_NOINTR.
>
> As this flag is only set during mount or reconnect. It is a problem
> for MP? Is it modified in a non-initialization path?
>
> bluhm
>
Sorry for non clean explanation.
This time solock() protects `sb_flags', so the following SB_NOINTR flag
check is fine:
sblock(struct socket *so, struct sockbuf *sb, int flags)
{
int error, prio = PSOCK;
soassertlocked(so);
if ((sb->sb_flags & SB_LOCK) == 0) {
sb->sb_flags |= SB_LOCK;
return (0);
}
if ((flags & SBL_WAIT) == 0)
return (EWOULDBLOCK);
if (!(flags & SBL_NOINTR || sb->sb_flags & SB_NOINTR))
prio |= PCATCH;
However, solock() will not protect `sb_flags' for standalone sblock().
It is not yet implemented, so here is the hypothetical sblock(). I
suppose the `sb_lock' rwlock(9) will protect the whole sockbuf data
include `sb_flags'. And there is the problem. As you can see, the
SB_NOINTR check presceeds the `sb_lock' acquisition, because the
rw_enter(9) behaviour depends on this flag.
sblock(struct sockbuf *sb, int flags)
{
int lockflags = RW_WRITE;
if (flags & SBL_NOINTR || sb->sb_flags & SB_NOINTR)
lockflags |= RW_INTR;
if ((flags & SBL_WAIT) == 0)
lockflags |= RW_NOSLEEP;
return rw_enter(&sb->sb_lock, lockflags);
}
I the case, when SB_NOINTR flag is immutable, there is no problem with
this unlocked check. Since this flag is really immutable and it's only
cleanup is the null op, I proposed to remove the flag cleanup.
However, this is not the only solution. This time this flag is not true
for sblock() because you always need to acquire solock() before. The
solock() acquisition is always uninterruptible, so ^C will not work. But
we assume the rwlock(9) acquisition doesn't take significant time,
right?
So, hypothetical sblock() could avoid the SB_NOINTR check. We have
SBL_NOINTR passed with `flags' for the sorflush() cases. This flag is
significant for sbwait(), but with the socket buffers standalone
locking, obviously sblock() should be taken before sbwait(), so the
`sb_flags' check will be protected:
sblock(struct sockbuf *sb, int flags)
{
int lockflags = RW_WRITE;
if (flags & SBL_NOINTR)
lockflags |= RW_INTR;
if ((flags & SBL_WAIT) == 0)
lockflags |= RW_NOSLEEP;
return rw_enter(&sb->sb_lock, lockflags);
}
sbwait(struct sockbuf *sb)
{
int prio = (sb->sb_flags & SB_NOINTR) ? PSOCK : PSOCK | PCATCH;
sbassertlocked(sb);
return rwsleep_nsec(&sb->sb_cc, &sb->sb_lock, prio, "sbwait",
sb->sb_timeo_nsecs);
}
Of course this is hypothetical. Also, SB_NOINTR flag has sense for TCP
and UDP sockets only. I don't think they will be the first sockets with
standalone locking ability for buffers. The unlocked SB_NOINTR flag
check is fine for all other sockets, so it could be left as is for now.
