From owner-svn-src-all@FreeBSD.ORG Mon Apr 18 20:17:01 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 60441106566C; Mon, 18 Apr 2011 20:17:01 +0000 (UTC) (envelope-from mdf356@gmail.com) Received: from mail-wy0-f182.google.com (mail-wy0-f182.google.com [74.125.82.182]) by mx1.freebsd.org (Postfix) with ESMTP id 9349D8FC25; Mon, 18 Apr 2011 20:17:00 +0000 (UTC) Received: by wyf23 with SMTP id 23so5226367wyf.13 for ; Mon, 18 Apr 2011 13:16:59 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:sender:in-reply-to:references:date :x-google-sender-auth:message-id:subject:from:to:cc:content-type :content-transfer-encoding; bh=0zqWqd7xybPfBEgPx46OpwEgIhAK87x7TjPdure9DiM=; b=WBxUrJDFNELTHZ0dk1d2thIQZSTaO8kyTIAaULxYPqVx2Q9hNwGVPWhZUIPEpFOQU+ 2Sa7GAwyB9ziuyNCkHOvjSXvzi5vmdHz/q1cdALO/9CtUa52OfjB+ZMQP2epwznYkW8Y +YudWNP3RzuGkhCCwRHTmg67mL9PIGVagJnpM= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:sender:in-reply-to:references:date :x-google-sender-auth:message-id:subject:from:to:cc:content-type :content-transfer-encoding; b=T41se7nst1uuAX5P5r/9+GwIE78oi4VhV3r/xPtWXfaq7hyT01D1BEVPNKOHMNQ1Hw wuBI35tVdTie0MRSL12czhDn6TrqnO0vg587m8PWLtiaAUU91HUvku+daxfm8nzTMV7K qtQwNwB7XGV+gkfDKjlG8TE07Rqz5Cv6eK0R4= MIME-Version: 1.0 Received: by 10.216.134.207 with SMTP id s57mr5348388wei.25.1303156331275; Mon, 18 Apr 2011 12:52:11 -0700 (PDT) Sender: mdf356@gmail.com Received: by 10.216.9.67 with HTTP; Mon, 18 Apr 2011 12:52:11 -0700 (PDT) In-Reply-To: <20110418194750.GD3097@garage.freebsd.pl> References: <201104181632.p3IGWM5v077720@svn.freebsd.org> <20110418192810.GX48734@deviant.kiev.zoral.com.ua> <20110418194750.GD3097@garage.freebsd.pl> Date: Mon, 18 Apr 2011 12:52:11 -0700 X-Google-Sender-Auth: g8t2ffNqCWDfmdEIVZ8rTjyz89g Message-ID: From: mdf@FreeBSD.org To: Pawel Jakub Dawidek Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Cc: Kostik Belousov , svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r220791 - in head: lib/libc/sys sys/compat/freebsd32 sys/kern sys/sys 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: Mon, 18 Apr 2011 20:17:01 -0000 On Mon, Apr 18, 2011 at 12:47 PM, Pawel Jakub Dawidek wro= te: > On Mon, Apr 18, 2011 at 10:28:10PM +0300, Kostik Belousov wrote: >> > + =A0 if (offset + len > vap->va_size) { >> > + =A0 =A0 =A0 =A0 =A0 VATTR_NULL(vap); >> > + =A0 =A0 =A0 =A0 =A0 vap->va_size =3D offset + len; >> > + =A0 =A0 =A0 =A0 =A0 error =3D VOP_SETATTR(vp, vap, td->td_ucred); >> > + =A0 =A0 =A0 =A0 =A0 if (error !=3D 0) >> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto out; >> > + =A0 } >> I still do not see a reason to do VOP_SETATTR() there. VOP_WRITE() will >> do auto-extend as needed. Also, see below. > > Yeah, also when we extend file size we could skip reading zeros. > >> > + =A0 =A0 =A0 =A0 =A0 if (offset < vap->va_size) { > [...] >> > + =A0 =A0 =A0 =A0 =A0 } else { >> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 bzero(buf, cur); >> > + =A0 =A0 =A0 =A0 =A0 } >> Wouldn't VOP_SETATTR() at the start of the function mostly prevent >> this bzero from executing ? > > Once we drop the vnode lock, the file size can change under us, no? Yes, which is why the VOP_GETATTR is re-run. The SETATTR doesn't need to be re-run, since the purpose was just to check that offset + len is less than the filesystem's maximum supported file size. >> I estimated what it would take to do the optimized implementation for UF= S, >> and I think that the following change would allow to lessen the code >> duplication much. >> >> What if the vnode lock drop and looping be handled by the syscall, inste= ad >> of the vop implementation ? In other words, allow the VOP_ALLOCATE() >> to =A0allocate less then requested, and return the allocated amount to >> the caller. The loop would be centralized then, freeing fs from doing >> the dance. Also, if fs considers that suitable, it would do a whole >> allocation in one run. > > I'd still go with SEEK_DATA/SEEK_HOLE loop as I suggested on arch@. > If you would like to spend time on it, having SEEK_DATA/SEEK_HOLE > support in UFS would be beneficial for other purposes too. Well, if we had this functionality it could be used. I'd like the framework but the filesystems need modification to support it. We want it for OneFS at Isilon so eventually when I get to this part of the project I'll have some wrapper code if someone doesn't get there first. Cheers, matthew