From owner-freebsd-hackers@FreeBSD.ORG Mon Aug 25 08:35:45 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 D6793EB4 for ; Mon, 25 Aug 2014 08:35:45 +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 5CC773FE4 for ; Mon, 25 Aug 2014 08:35:45 +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 s7P8ZdvS091362 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Mon, 25 Aug 2014 11:35:39 +0300 (EEST) (envelope-from kostikbel@gmail.com) DKIM-Filter: OpenDKIM Filter v2.9.2 kib.kiev.ua s7P8ZdvS091362 Received: (from kostik@localhost) by tom.home (8.14.9/8.14.9/Submit) id s7P8ZdkH091361; Mon, 25 Aug 2014 11:35:39 +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 11:35:39 +0300 From: Konstantin Belousov To: Mateusz Guzik , freebsd-hackers@freebsd.org Subject: Re: atomic_load_acq_int in sequential_heuristic Message-ID: <20140825083539.GB2737@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> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="s24z9xI8tqkHlhZq" Content-Disposition: inline In-Reply-To: <20140825081526.GB14344@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 08:35:45 -0000 --s24z9xI8tqkHlhZq Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Aug 25, 2014 at 10:15:26AM +0200, Mateusz Guzik wrote: > On Mon, Aug 25, 2014 at 10:34:04AM +0300, Konstantin Belousov wrote: > [..] > > Two notes. First, please add a comment explaining which other part > > of the code is locked against in F_READAHEAD switch case. Second, > > should the vnode lock cover the FRDAHEAD reset case too, at least > > for consistency ? >=20 > I'll start with a side thing: >=20 > diff --git a/sys/kern/vfs_vnops.c b/sys/kern/vfs_vnops.c > index 98823f3..2c3df2b 100644 > --- a/sys/kern/vfs_vnops.c > +++ b/sys/kern/vfs_vnops.c > @@ -365,7 +365,7 @@ vn_open_vnode(struct vnode *vp, int fmode, struct ucr= ed *cred, > fp->f_ops=3D &badfileops; > return (error); > } > - fp->f_flag |=3D FHASLOCK; > + atomic_set_int(&fp->f_flag, FHASLOCK); > } > if (fmode & FWRITE) { > VOP_ADD_WRITECOUNT(vp, 1); Ok. >=20 > And now for the patch in question: >=20 > Yes, _rel is not necessary. In fact, the loop is not necessary either. It > would be useful if more than one flag was altered. >=20 > diff --git a/sys/kern/kern_descrip.c b/sys/kern/kern_descrip.c > index 7abdca0..433b809 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 > @@ -762,23 +761,21 @@ kern_fcntl(struct thread *td, int fd, int cmd, intp= tr_t arg) > } > if (arg >=3D 0) { > vp =3D fp->f_vnode; > - error =3D vn_lock(vp, LK_SHARED); > + /* > + * Exclusive lock synchronizes against > + * sequential_heuristic(). > + */ > + error =3D vn_lock(vp, LK_EXCLUSIVE); > 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 (arg + 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, FHASLOCK); You misspelled FRDAHEAD as FHASLOCK, below as well. Was this tested ? > + 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 ? > } > fdrop(fp, td); > break; > 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 > --=20 > Mateusz Guzik --s24z9xI8tqkHlhZq Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBAgAGBQJT+vVbAAoJEJDCuSvBvK1BjJkP/iSWiYgVq2wE+jFjsacCXfPc E/baLYWpOMNXJ51p8QRAEGgkrlACVZdloIpbdNAvdil7hTCR5D4KpY+Z1o+AzHya FExWKGpJShysRTOIlT0w/k6fH6XtmtkYD4Ai9eQHrlrs0r8wLKxWruTGfhd5HMU7 OZmkz61OGD69dvJg31+ONBKFFku8zqNWR112Bb7b7ql1n3hDSDWFSpMQMVtGTMFM 5roHPPOUG8E5UwhtRSLr0aLaANSVFQoJjep3KE8l+25CF0OtcRy4rcRBIq1ieiff BxhfrgV4fx0OzrTB2Dy9jVyMsBgWRho/IhB/S5Lllf6ioYjTjzMMdL4U1l7WQWRh 2gt1+wT2S+HsWYlClfmInNpCRf1503bV/uIFEw+6vg1frWIKjyiAUZrL//xHllE5 ZqRtAevtsduUqRYly+2Qw70qS/H+oUlXHj5zVmTjRPnIVoNp1/JUBSjDS2nKpfBf /XUhJfk+O2bE6EkcBMdBUPGbu2lyZ/0dZFgQWQrM66tn8e3d89+xvhF5VHmwp45p QAUhF6FocH2YXc0scGHff4XzE5aWWTRiZm/x7cT7mLfMwy8WfKAvJhHOt4RyPlZF WZxK85Js542BeTbG3s8OvlDgbdYrvYAADJ5qZ6zvDRUp2P1du0TJ4dq0oVKZMN0m kRC9NsP+MJfE41Js20HW =bfKF -----END PGP SIGNATURE----- --s24z9xI8tqkHlhZq--