On Thu, 2023-11-16 at 18:18 +0000, Chuck Lever III wrote: > > > On Nov 16, 2023, at 12:11 PM, Benjamin Coddington <bcodding@xxxxxxxxxx> wrote: > > > > I ran into this old problem again recently, last discussed here: > > https://lore.kernel.org/linux-nfs/F738DB31-B527-4048-94A7-DBF00533F8C1@xxxxxxxxxx/#t > > > > Problem is that clients can easily issue enough WRITEs that end up in > > > > __break_lease > > xfs_break_leased_layouts > > ... > > nfsd_vfs_write > > ... > > svc_process > > svc_recv > > nfsd > > > > .. so that all the knfsds are there, and nothing can process the > > LAYOUTRETURN. > > > > I'm pretty sure we can make the linux client a bit smarter about it (I saw > > one LAYOUTGET and one conflicting WRITE in the same TCP segment, in that > > order). > > > > But what can knfsd do to defend itself? One thing that might help would be make the nfsd threadpool more dynamic. If it could just spin up another thread, we'd be OK. Maybe the server could keep an emergency thread around that is just for processing LAYOUTRETURN/DELEGRETURN (maybe also CLOSE, etc.) calls? Sometimes these ops are mixed into compounds with other sorts of calls though, so we'd need to deal with that somehow. > > If nfsd threads are waiting indefinitely, that's a potential DoS > vector. Ideally the thread should preserve the waiting request > somehow (or return NFS4ERR_DELAY, maybe?). At some later point > when the lease conflict is resolved, the requests can be reprocessed. > > That's my naive 800,000 ft view. > Yeah, that would probably be ideal. xfs_break_leased_layouts is pretty heavy handed right now. It currently does this: while ((error = break_layout(inode, false)) == -EWOULDBLOCK) { xfs_iunlock(ip, *iolock); *did_unlock = true; error = break_layout(inode, true); *iolock &= ~XFS_IOLOCK_SHARED; *iolock |= XFS_IOLOCK_EXCL; xfs_ilock(ip, *iolock); } So you can always get stuck in that inner break_layout call. We could try to use IOCB_NOWAIT for I/Os coming from nfsd, which would prevent that, but that seems like it could change the I/O behavior in other ways we don't want. It's not clear to me how that would work alongside IOCB_SYNC anyway. It's not immediately clear to me how we'd do this with the existing IOCB_* flags, so we might need a new one (IOCB_NFSD?). Then, we could just make sure that break_layout call is always nonblocking if that flag is set. -- Jeff Layton <jlayton@xxxxxxxxxx>
