Skip site navigation (1)Skip section navigation (2)
Date:      Mon, 18 Apr 2011 12:52:11 -0700
From:      mdf@FreeBSD.org
To:        Pawel Jakub Dawidek <pjd@freebsd.org>
Cc:        Kostik Belousov <kostikbel@gmail.com>, 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
Message-ID:  <BANLkTi=8NtVxci_L2krtFsuSj2HA0Q_Wgw@mail.gmail.com>
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>

next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, Apr 18, 2011 at 12:47 PM, Pawel Jakub Dawidek <pjd@freebsd.org> 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



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?BANLkTi=8NtVxci_L2krtFsuSj2HA0Q_Wgw>