Date: Mon, 25 Aug 2014 11:35:39 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: Mateusz Guzik <mjguzik@gmail.com>, freebsd-hackers@freebsd.org Subject: Re: atomic_load_acq_int in sequential_heuristic Message-ID: <20140825083539.GB2737@kib.kiev.ua> In-Reply-To: <20140825081526.GB14344@dft-labs.eu> 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>
next in thread | previous in thread | raw e-mail | index | archive | help
--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 <mjguzik gmail.com>
--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--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140825083539.GB2737>