Date: Sat, 13 Apr 2019 08:23:54 -0700 From: Kirk McKusick <mckusick@mckusick.com> To: Konstantin Belousov <kostikbel@gmail.com> Cc: Alan Somers <asomers@freebsd.org>, FreeBSD Hackers <freebsd-hackers@freebsd.org> Subject: Re: When can a struct buf's b_lblkno field by < 0 ? Message-ID: <201904131523.x3DFNsu4091963@chez.mckusick.com> In-Reply-To: <20190412154117.GD1923@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
> Date: Fri, 12 Apr 2019 18:41:17 +0300
> From: Konstantin Belousov <kostikbel@gmail.com>
> To: Alan Somers <asomers@freebsd.org>
> Cc: FreeBSD Hackers <freebsd-hackers@freebsd.org>,
> Kirk McKusick <mckusick@mckusick.com>
> Subject: Re: When can a struct buf's b_lblkno field by < 0 ?
> =
> On Fri, Apr 12, 2019 at 09:10:58AM -0600, Alan Somers wrote:
>> In struct buf, b_lblkno is documented as "Logical block number". I
>> would expect that to always be nonnegative. However, vtruncbuf loops
>> through a list of buffers, skipping those where "bp->b_lblkno > 0".
>> Maybe that's just an awkward way of writing "do something for the
>> buffer where b_lblkno =3D=3D 0", but SVN archaeology suggests otherwise=
.
>> Before r112182, the code looked like this, implying that the b_lblkno
>> could actually be negative:
>> =
>> if ((bp->b_flags & B_DELWRI) && (bp->b_lblkno < 0)) {
>> =
>> Does anybody know under what circumstances that field might be
>> negative?
> =
> Yes, somebody know. Better list for such discussions is fs@.
> =
> It is up to the filesystem to use any values of b_lblkno as it finds
> suitable. For instance, for UFS lblknos -1 and -2 are used for the
> blocks carrying the extended attributes of inode. Lower values
> are used to cache indirect block' pages in the normal vnode page queue,
> so that the pages effectively appear as having very large page indexes.
> =
>> Also, was r112182 a correct change? It appears to have
>> negated "<" and gotten ">", neglecting the "=3D=3D" case.
>> https://svnweb.freebsd.org/base/head/sys/kern/vfs_subr.c?r1=3D112182&r2=
=3D112181&pathrev=3D112182
> =
> This is innocent, in worst case it results in unneeded syncing. But I
> wonder why do we still have this loop for 'if (length > 0)' case. I do
> not quite follow the purpose of syncing all indirect and extended blocks
> on truncation. It might somewhat help SU to ensure that dependencies
> carried by indirect blocks are flushed timely after truncation, but this
> is too vague.
> =
> I added Kirk who may know more history there.
I don't have much to add to kib's commentary. The change in -r112182
was not meant to have functional change, just reduce indentation and
get rid of unnecessary code. The change should have been to =
(bp->b_lblkno >=3D 0), but the effect is that if logical data block 0
is in the cache and dirty it will be unnecessarily written. At that
time all partial truncations were done synchronously, hence the
flushing of all the meta-data. With the additional of journaled soft
updates, it became possible to do partial truncations asynchronously.
Kirk McKusick
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201904131523.x3DFNsu4091963>