From owner-svn-src-all@FreeBSD.ORG Wed Apr 20 16:17:13 2011 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 1F9961065673; Wed, 20 Apr 2011 16:17:13 +0000 (UTC) (envelope-from pawel@dawidek.net) Received: from mail.garage.freebsd.pl (60.wheelsystems.com [83.12.187.60]) by mx1.freebsd.org (Postfix) with ESMTP id 543258FC1D; Wed, 20 Apr 2011 16:17:11 +0000 (UTC) Received: by mail.garage.freebsd.pl (Postfix, from userid 65534) id EE76345C89; Wed, 20 Apr 2011 18:17:09 +0200 (CEST) Received: from localhost (89-73-195-149.dynamic.chello.pl [89.73.195.149]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mail.garage.freebsd.pl (Postfix) with ESMTP id A99DE45685; Wed, 20 Apr 2011 18:17:04 +0200 (CEST) Date: Wed, 20 Apr 2011 18:16:55 +0200 From: Pawel Jakub Dawidek To: Rick Macklem Message-ID: <20110420161655.GA1907@garage.freebsd.pl> References: <20110420054655.GD1826@garage.freebsd.pl> <630616771.329277.1303301372199.JavaMail.root@erie.cs.uoguelph.ca> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="mP3DRpeJDSE+ciuQ" Content-Disposition: inline In-Reply-To: <630616771.329277.1303301372199.JavaMail.root@erie.cs.uoguelph.ca> X-OS: FreeBSD 9.0-CURRENT amd64 User-Agent: Mutt/1.5.21 (2010-09-15) X-Spam-Checker-Version: SpamAssassin 3.0.4 (2005-06-05) on mail.garage.freebsd.pl X-Spam-Level: X-Spam-Status: No, score=-0.6 required=4.5 tests=BAYES_00,RCVD_IN_SORBS_DUL autolearn=no version=3.0.4 Cc: svn-src-head@freebsd.org, Rick Macklem , svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r220877 - head/sys/fs/nfsclient X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 20 Apr 2011 16:17:13 -0000 --mP3DRpeJDSE+ciuQ Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Apr 20, 2011 at 08:09:32AM -0400, Rick Macklem wrote: > > > + tmp_off =3D uio->uio_offset + uio->uio_resid; > > > + mtx_lock(&nmp->nm_mtx); > > > + if (tmp_off > nmp->nm_maxfilesize || tmp_off < uio->uio_offset) { > > > + mtx_unlock(&nmp->nm_mtx); > > > return (EFBIG); > > > + } > > > + mtx_unlock(&nmp->nm_mtx); > >=20 > > I don't think you need the lock to protect nm_maxfilesize. Can it > > change > > from under us? My guess is that it is set on mount time and is not > > modified afterwards. > >=20 > Good question. > For NFSv3 - it is only modified by the first fsinfo RPC and that normally > happens at mount time, as you guessed above. (This is consistent with > RFC1813, which defines the fsinfo RPC as getting non-volatile informa= tion.) > For NFSv4 - it gets it each time VFS_STATFS() happens. I am not sure that > this is correct, but I don't know of anywhere in RFC3530 where it sta= tes > that this attribute will not change. In practice, I suspect that serv= ers > seldom, if ever, change it. >=20 > So, it is unlikely to change and I'd be comfortable taking the mutex lock > off the check for it, if others are? (As you might be aware, I started a > thread on hackers-freebsd@ where my question was basically "do you need to > mutex lock when you read a global variable". My main concern there was a > case that I'm working on w.r.t. forced dismounts. jhb@ suggested that he > thinks it is good practice to always lock, to play it safe. At least that > was my interpretation?) This is not that easy, I'm afraid. You need to ask yourself a question what you are trying to protect from. Here, the mutex only guarantees to have consistent view of the nm_maxfilesize field. For example if this field modification wouldn't be atomic you would need the mutex to ensure that the value is correct. Imagine a situation where it is modifed not by simple 'a =3D b', but by something like this: mtx_lock(&nmp->nm_mtx); nmp->nm_maxfilesize =3D some_32bit_array[0]; nmp->nm_maxfilesize |=3D some_32bit_array[1] << 32; mtx_unlock(&nmp->nm_mtx); To read that properly you need a mutex to ensure you won't read the value between those two operations. If it is not the case - its modification is atomic and reading it is atomic then you don't need mutex to read the value as it will always be consistent. The question is what will happen if it changes after you read it. thread0 thread1 ------- ------- mtx_lock(&nmp->nm_mtx); nmp->nm_maxfilesize =3D 8192; mtx_unlock(&nmp->nm_mtx); mtx_lock(&nmp->nm_mtx); if (tmp_off > nmp->nm_maxfilesize) { mtx_unlock(&nmp->nm_mtx); return (EFBIG); } mtx_unlock(&nmp->nm_mtx); mtx_lock(&nmp->nm_mtx); nmp->nm_maxfilesize =3D 2048; mtx_unlock(&nmp->nm_mtx); Now, if tmp_off is 4096 what will happen if you have a race like the above? Is it critical? Then you need to protect with this mutex as well. --=20 Pawel Jakub Dawidek http://www.wheelsystems.com FreeBSD committer http://www.FreeBSD.org Am I Evil? Yes, I Am! http://yomoli.com --mP3DRpeJDSE+ciuQ Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.14 (FreeBSD) iEYEARECAAYFAk2vBvYACgkQForvXbEpPzT+/QCdFqw13GVrE7j8yGvnYJ/LaYlj KVsAnjSrq6O6PvHuGcaOJDuNOMEoc94G =dG5B -----END PGP SIGNATURE----- --mP3DRpeJDSE+ciuQ--