From owner-freebsd-hackers@FreeBSD.ORG Mon Aug 25 08:15:34 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 014BEAA2 for ; Mon, 25 Aug 2014 08:15:33 +0000 (UTC) Received: from mail-wi0-x22a.google.com (mail-wi0-x22a.google.com [IPv6:2a00:1450:400c:c05::22a]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 8A3183DE7 for ; Mon, 25 Aug 2014 08:15:33 +0000 (UTC) Received: by mail-wi0-f170.google.com with SMTP id f8so3326837wiw.1 for ; Mon, 25 Aug 2014 01:15:31 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:cc:subject:message-id:mail-followup-to:references :mime-version:content-type:content-disposition:in-reply-to :user-agent; bh=0ZkIswYknQxuexOhjIV44iBfgdpdKJshLLWXvWPwA3g=; b=ckHXMQT4Xxzdxicd5JM5p/P2JlwZG1iulPRCvHxb/oXOQ1Ms6uUGWcRdQ6TsIjrAxz y56gt39mDMgUkXYoRMvoW+QeH4VoJ3Wzpt7I2YZhRmwfBdt5kjcBISWdElge0N3y94GG GTa5uqnqcCNgcH+PFDqR1aoL0h+SH2/garwvMnsnqAgXD7ZFBdVZXgSe08VpwGhPJ1Ka lCCiVM7zHJ1+E9+rW2EMEZye4yQ87WKrbI8pT1SWvJWsm5OIlbc11xZO+8muhS0D/f+u 7lypaRbwDR1rkc6cibgZJYi+IYOw1beRpl+25WQh1RoLqnbmdSflBrw0ah0lUe3z/vzQ 7dRg== X-Received: by 10.194.187.4 with SMTP id fo4mr21314192wjc.35.1408954531693; Mon, 25 Aug 2014 01:15:31 -0700 (PDT) Received: from dft-labs.eu (n1x0n-1-pt.tunnel.tserv5.lon1.ipv6.he.net. [2001:470:1f08:1f7::2]) by mx.google.com with ESMTPSA id bk9sm29253416wib.7.2014.08.25.01.15.30 for (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Mon, 25 Aug 2014 01:15:30 -0700 (PDT) Date: Mon, 25 Aug 2014 10:15:26 +0200 From: Mateusz Guzik To: Konstantin Belousov Subject: Re: atomic_load_acq_int in sequential_heuristic Message-ID: <20140825081526.GB14344@dft-labs.eu> Mail-Followup-To: Mateusz Guzik , Konstantin Belousov , freebsd-hackers@freebsd.org 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> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20140825073404.GZ2737@kib.kiev.ua> User-Agent: Mutt/1.5.21 (2010-09-15) Cc: freebsd-hackers@freebsd.org 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:15:34 -0000 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 ? I'll start with a side thing: 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 ucred *cred, fp->f_ops= &badfileops; return (error); } - fp->f_flag |= FHASLOCK; + atomic_set_int(&fp->f_flag, FHASLOCK); } if (fmode & FWRITE) { VOP_ADD_WRITECOUNT(vp, 1); And now for the patch in question: Yes, _rel is not necessary. In fact, the loop is not necessary either. It would be useful if more than one flag was altered. 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; @@ -762,23 +761,21 @@ kern_fcntl(struct thread *td, int fd, int cmd, intptr_t arg) } if (arg >= 0) { vp = fp->f_vnode; - error = vn_lock(vp, LK_SHARED); + /* + * Exclusive lock synchronizes against + * sequential_heuristic(). + */ + error = vn_lock(vp, LK_EXCLUSIVE); if (error != 0) { fdrop(fp, td); break; } bsize = fp->f_vnode->v_mount->mnt_stat.f_iosize; - VOP_UNLOCK(vp, 0); fp->f_seqcount = (arg + bsize - 1) / bsize; - do { - new = old = fp->f_flag; - new |= FRDAHEAD; - } while (!atomic_cmpset_rel_int(&fp->f_flag, old, new)); + atomic_set_int(&fp->f_flag, FHASLOCK); + VOP_UNLOCK(vp, 0); } else { - do { - new = old = fp->f_flag; - new &= ~FRDAHEAD; - } while (!atomic_cmpset_rel_int(&fp->f_flag, old, new)); + atomic_clear_int(&fp->f_flag, FHASLOCK); } 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) { - 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); /* -- Mateusz Guzik