From owner-freebsd-hackers@FreeBSD.ORG Mon Aug 25 11:10:12 2014 Return-Path: Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 231E48C9 for ; Mon, 25 Aug 2014 11:10:12 +0000 (UTC) Received: from kib.kiev.ua (kib.kiev.ua [IPv6:2001:470:d5e7:1::1]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id B71653E99 for ; Mon, 25 Aug 2014 11:10:11 +0000 (UTC) Received: from tom.home (kib@localhost [127.0.0.1]) by kib.kiev.ua (8.14.9/8.14.9) with ESMTP id s7PBA16S026256 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Mon, 25 Aug 2014 14:10:01 +0300 (EEST) (envelope-from kostikbel@gmail.com) DKIM-Filter: OpenDKIM Filter v2.9.2 kib.kiev.ua s7PBA16S026256 Received: (from kostik@localhost) by tom.home (8.14.9/8.14.9/Submit) id s7PBA1aj026255; Mon, 25 Aug 2014 14:10:01 +0300 (EEST) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: tom.home: kostik set sender to kostikbel@gmail.com using -f Date: Mon, 25 Aug 2014 14:10:01 +0300 From: Konstantin Belousov To: Mateusz Guzik , freebsd-hackers@freebsd.org Subject: Re: atomic_load_acq_int in sequential_heuristic Message-ID: <20140825111000.GC2737@kib.kiev.ua> References: <20140824115729.GC2045@dft-labs.eu> <20140824162331.GW2737@kib.kiev.ua> <20140824164236.GX2737@kib.kiev.ua> <20140825005659.GA14344@dft-labs.eu> <20140825073404.GZ2737@kib.kiev.ua> <20140825081526.GB14344@dft-labs.eu> <20140825083539.GB2737@kib.kiev.ua> <20140825091056.GC14344@dft-labs.eu> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="tTm07/aDcFLy+pwu" Content-Disposition: inline In-Reply-To: <20140825091056.GC14344@dft-labs.eu> User-Agent: Mutt/1.5.23 (2014-03-12) X-Spam-Status: No, score=-2.0 required=5.0 tests=ALL_TRUSTED,BAYES_00, DKIM_ADSP_CUSTOM_MED,FREEMAIL_FROM,NML_ADSP_CUSTOM_MED autolearn=no autolearn_force=no version=3.4.0 X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on tom.home X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 25 Aug 2014 11:10:12 -0000 --tTm07/aDcFLy+pwu Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Aug 25, 2014 at 11:10:56AM +0200, Mateusz Guzik wrote: > On Mon, Aug 25, 2014 at 11:35:39AM +0300, Konstantin Belousov wrote: > > > + atomic_set_int(&fp->f_flag, FHASLOCK); > > You misspelled FRDAHEAD as FHASLOCK, below as well. > > Was this tested ? > >=20 >=20 > Oops, damn copy-pasto. Sorry. >=20 > > > + VOP_UNLOCK(vp, 0); > > > } else { > > > - do { > > > - new =3D old =3D fp->f_flag; > > > - new &=3D ~FRDAHEAD; > > > - } while (!atomic_cmpset_rel_int(&fp->f_flag, old, new)); > > > + atomic_clear_int(&fp->f_flag, FHASLOCK); > > So what about extending the vnode lock to cover the flag reset ? > >=20 >=20 > Sure. >=20 > So this time I tested it properly and found out it is impossible to > disable the hint. The test is: >=20 > -1 is truncated and then read from intptr_t which yields a big positive > number instead. Other users in the function use int tmp to work around > this issue. Could you provide me with the test case which demonstrates the problem ? The fcntl(2) prototype in sys/fcntl.h is variadic, so int arg argument is not promoted. On the other hand, syscalls.master declares arg as long. Did you tried to pass -1L as third argument to disable ? >=20 > diff --git a/sys/kern/kern_descrip.c b/sys/kern/kern_descrip.c > index 7abdca0..774f647 100644 > --- a/sys/kern/kern_descrip.c > +++ b/sys/kern/kern_descrip.c > @@ -760,7 +760,8 @@ kern_fcntl(struct thread *td, int fd, int cmd, intptr= _t arg) > error =3D EBADF; > break; > } > - if (arg >=3D 0) { > + tmp =3D arg; > + if (tmp >=3D 0) { > vp =3D fp->f_vnode; > error =3D vn_lock(vp, LK_SHARED); > if (error !=3D 0) { > @@ -769,7 +770,7 @@ kern_fcntl(struct thread *td, int fd, int cmd, intptr= _t arg) > } > bsize =3D fp->f_vnode->v_mount->mnt_stat.f_iosize; > VOP_UNLOCK(vp, 0); > - fp->f_seqcount =3D (arg + bsize - 1) / bsize; > + fp->f_seqcount =3D (tmp + bsize - 1) / bsize; > do { > new =3D old =3D fp->f_flag; > new |=3D FRDAHEAD; >=20 > Then the patch in question: >=20 > diff --git a/sys/kern/kern_descrip.c b/sys/kern/kern_descrip.c > index 774f647..4efadb0 100644 > --- a/sys/kern/kern_descrip.c > +++ b/sys/kern/kern_descrip.c > @@ -476,7 +476,6 @@ kern_fcntl(struct thread *td, int fd, int cmd, intptr= _t arg) > struct vnode *vp; > cap_rights_t rights; > int error, flg, tmp; > - u_int old, new; > uint64_t bsize; > off_t foffset; > =20 > @@ -760,27 +759,25 @@ kern_fcntl(struct thread *td, int fd, int cmd, intp= tr_t arg) > error =3D EBADF; > break; > } > + vp =3D fp->f_vnode; > + /* > + * Exclusive lock synchronizes against > + * sequential_heuristic(). I would also add a sentence that we care about f_seqcount update in seq_heur(). Another place to add the locking annotation is the struct file in sys/file.h. Now f_seqcount is 'protected' by the vnode lock. I am not sure how to express the locking model shortly. > + */ > + error =3D vn_lock(vp, LK_EXCLUSIVE); > + if (error !=3D 0) { > + fdrop(fp, td); > + break; > + } > tmp =3D arg; > if (tmp >=3D 0) { > - vp =3D fp->f_vnode; > - error =3D vn_lock(vp, LK_SHARED); > - if (error !=3D 0) { > - fdrop(fp, td); > - break; > - } > bsize =3D fp->f_vnode->v_mount->mnt_stat.f_iosize; > - VOP_UNLOCK(vp, 0); > fp->f_seqcount =3D (tmp + bsize - 1) / bsize; > - do { > - new =3D old =3D fp->f_flag; > - new |=3D FRDAHEAD; > - } while (!atomic_cmpset_rel_int(&fp->f_flag, old, new)); > + atomic_set_int(&fp->f_flag, FRDAHEAD); > } else { > - do { > - new =3D old =3D fp->f_flag; > - new &=3D ~FRDAHEAD; > - } while (!atomic_cmpset_rel_int(&fp->f_flag, old, new)); > + atomic_clear_int(&fp->f_flag, FRDAHEAD); > } > + VOP_UNLOCK(vp, 0); > fdrop(fp, td); > break; > =20 > diff --git a/sys/kern/vfs_vnops.c b/sys/kern/vfs_vnops.c > index f1d19ac..98823f3 100644 > --- a/sys/kern/vfs_vnops.c > +++ b/sys/kern/vfs_vnops.c > @@ -438,7 +438,8 @@ static int > sequential_heuristic(struct uio *uio, struct file *fp) > { > =20 > - if (atomic_load_acq_int(&(fp->f_flag)) & FRDAHEAD) > + ASSERT_VOP_LOCKED(fp->f_vnode, __func__); > + if (fp->f_flag & FRDAHEAD) > return (fp->f_seqcount << IO_SEQSHIFT); > =20 > /* > --=20 > Mateusz Guzik --tTm07/aDcFLy+pwu Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBAgAGBQJT+xmIAAoJEJDCuSvBvK1BmZwP/2LKzDdUErkIZr4iCkFRDLUr xmEngq42kKdrcDPSKjI6j9n7n8k5kSXMl93h2G3t9Q/X526bMmXzaP3gUnLBxJOc ahA2UxBlIT8FC9GxdOFy/m1bBq+i+X1+dnrDtImyYYN2Wh0b6Xw/QvQtiji58Qh1 gGDX/cFENJcXcgEBnZgMNyVeHxjnBfRF1F6cEGVExxLbg0b5M5VmF8oxTQnlxyKe 1GV1PYC2CiFCDkx/lFB85hRjYCTxXyak0sJIH6ySkXTU1vxlcKfWTtWqz6xRG6H8 HHrkLWLID5gdhSK97u7z0IcYSY1ZvSMu3/d7FpeHnS/hgsxg0GtPdkiZhNwTlgoJ fTsiZuvkUBTTKYnhLzGvxkbwaVaOS0zAKsiONynscKXmKIyELRiUSL/mxi83stPw /P7rvGb72rCt5W2ezWh9OjtKvNJVlaHTRh7YWk3213Bgs9VqkSN4dYceHpdkB+XN jOD5T54gB8vLeZYVr4eopju1RLT77O/kKBCLnuuf+hlPv6kt62VZOJrE0P6WOc6m 8jluxqU0//wRx83gADmnHX4P4hix4C20qw23gDSGubCaBy4qzCDiewyyIbyAlM79 QTKIJnh338HNTjQz1mSbpcPEAMof2ZFRjYDTknodj1LyrQp83QQHDKnSLF8EZJUR F3rRTJnkzUJIidBU7MeL =i1pV -----END PGP SIGNATURE----- --tTm07/aDcFLy+pwu--