From: Greg Banks <gnb@sgi.com>
Subject: Re: [RFC,PATCH 4/14] knfsd: has_wspace per transport
Date: Fri, 18 May 2007 14:05:09 +1000
Message-ID: <20070518040509.GC5104@sgi.com>
References: <20070516192211.GJ9626@sgi.com>
	<20070516211053.GE18927@fieldses.org>
	<20070517071202.GE27247@sgi.com>
	<17996.11983.278205.708747@notabene.brown>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Cc: "J. Bruce Fields" <bfields@fieldses.org>,
	Linux NFS Mailing List <nfs@lists.sourceforge.net>,
	Thomas Talpey <Thomas.Talpey@netapp.com>,
	Peter Leckie <pleckie@melbourne.sgi.com>
To: Neil Brown <neilb@suse.de>
In-Reply-To: <17996.11983.278205.708747@notabene.brown>
Sender: nfs-bounces@lists.sourceforge.net
Errors-To: nfs-bounces@lists.sourceforge.net

On Thu, May 17, 2007 at 08:30:39PM +1000, Neil Brown wrote:
> On Thursday May 17, gnb@sgi.com wrote:
> > On Wed, May 16, 2007 at 05:10:53PM -0400, J. Bruce Fields wrote:
> > > On Thu, May 17, 2007 at 05:22:11AM +1000, Greg Banks wrote:
> > > > +	set_bit(SOCK_NOSPACE, &svsk->sk_sock->flags);
> > > > +	if (required*2 > wspace) {
> > > > +		/* Don't enqueue while not enough space for reply */
> > > > +		dprintk("svc: socket %p  no space, %d*2 > %d, not enqueued\n",
> > > > +			svsk->sk_sk, required, wspace);
> > > > +		return 0;
> > > > +	}
> > > > +	clear_bit(SOCK_NOSPACE, &svsk->sk_sock->flags);
> > > > +	return 1;
> > > > +}
> > > 
> > > So, this is just my ignorance--why do the set and clear of SOCK_NOSPACE
> > > need to be ordered in the way they are?  (Why not just set once inside
> > > the if clause?)
> > 
> > I can't see a good reason for it, but I'm trying to minimise
> > perturbations to the logic.
> 
> Unfortunately, you actually perturbed the important bit... Or at
> least, the bit that I thought was important when I wrote it.
> 
> Previously, sk_stream_wspace(), or sock_wspace() would be called *after*
> SOCK_NOSPACE was set.  With your patch it is called *before*.
> 
> It is a fairly improbably race, but if the output queue flushed
> completely between calling XX_wspace and setting SOCK_NOSPACE, the
> sk_write_space callback might never get called.

Woops.  I'll fix that.

> And I gather by the fact that you test "->sko_has_wspace" that RDMA
> doesn't have such a function?

You gather correctly.

>  Do that mean that RDMA will never
> reject a write due to lack of space? 

No, it means that the current RDMA send code will block waiting
for space to become available.  That's right, nfsd threads block on
the network.  Steel yourself, there's worse to come.

> That seems unlikely.
> I would rather assume that every transport has a sko_has_wspace
> function... 

Ok, but for today the RDMA one will be

static int svc_rdma_has_wspace(struct svc_sock *svsk)
{
	return 1;
}

We might be able to pull the condition in the blocking logic out
of svc_rdma_send() out to implement an sko_has_wspace, but there's
something of an impedance mismatch.  The RDMA queue limit is expressed
in Work Requests not bytes, and the mapping between the two is not
precisely visible at the point when has_wspace is called.  I guess
we'd have to use an upper bound.

Greg.
-- 
Greg Banks, R&D Software Engineer, SGI Australian Software Group.
Apparently, I'm Bedevere.  Which MPHG character are you?
I don't speak for SGI.

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs