Date: Sat, 13 Apr 2019 18:43:40 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: Kirk McKusick <mckusick@mckusick.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: <20190413154340.GJ1923@kib.kiev.ua> In-Reply-To: <201904131523.x3DFNsu4091963@chez.mckusick.com> References: <20190412154117.GD1923@kib.kiev.ua> <201904131523.x3DFNsu4091963@chez.mckusick.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, Apr 13, 2019 at 08:23:54AM -0700, Kirk McKusick wrote:
> > 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 == 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 "==" case.
> >> https://svnweb.freebsd.org/base/head/sys/kern/vfs_subr.c?r1=112182&r2=112181&pathrev=112182
> >
> > 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 >= 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.
Do you agree with the statement that the last loop in vtruncbuf() is
useless ? Its removal could only make a difference for ffs_truncate(),
and there, I do not think that b(a)write() is enough to ensure that the
indirect buffers are clean, due to dependencies.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20190413154340.GJ1923>