On Wed, Oct 30, 2002 at 01:37:35AM -0500, Luben Tuikov wrote:
> Do you really like this:
>
> if (psync) {
> init_buffer(bh, callback, psync);
> } else {
> init_buffer(bh, callback, pb);
> }
>
> over this:
>
> init_buffer(bh, callback, psync ? : pb);
>
> Really? You like the first better?
No, I don't like both :) Just use evaluate psync twice
instead of the GNU-extension:
init_buffer(bh, callback, psync ? psync : pb);
> > The 2.4 MD code isn't exactly a good reference to copy from :)
> > 2.5 md code is a lot better, but that one deals only with bios,
> > not buffer_heads.
>
> As you have probably deduced, we're using the 2.4 series
> (stable release) kernel.
>
> If you haven't deduced this: we are using the 2.4 series
> (stable release) kernel.
Thanks Luben, I might not be a genius like you, but I can guess that you
use 2.4 when you submit patches against that tree. Which still doesn't make
the 2.4 md driver a good example.
> > It makes sense if you let the VM control the write out, but pagebuf
> > does the writeout explicit and the bit doesn't matter at all.
>
> So, you do admit that ``it makes sense ...'' -- so you agree with me.
> (My god -- did you?! You'll now be ousted from linux-xfs!)
>
> So, IOW, this would be *consistent* with the rest (VM, you mention).
> So, let's be consistent. Let's turn on the dirty bit. This will be
> consistent as if coming from the VM, you say... so the LL driver wouldn't
> be playing guessing games where the thing came from... So let's just be
> consistent.
No. Let me repeat: BH_dirty is an VM-internal state bit. Unless
you want VM-controlled writeback of the buffer (by putting in on the LRU
list) you should NOT set it.
> > Look at fs/buffer.c - usually we set it before submitting and clear
> > it if the I/O failed. And yes, this is confusing when you look
> > at it.
>
> Why are you repeating what I'm saying? You alrealy admit that
> it is confusing so what is the value and purpose of your post here?
>
> Why not just admit that this is the reversed logic, and have the
> io completion callback worry about the Uptodate bit.
I told you that this is the way the generic I/O path does it for a reason.
The pagebuf code doesn't need it and we could discuss two days whether it's
more intuitive to follow the most used and reviewed code or do what
people expect from other operating systems. I don't really care personally,
but I haveå strong feeling that changing it from whathever it was before is
rather useless..
> > Drivers should never ever look at BH_Dirty.
>
> (Can you guarantee that no such (stacking) drivers exist?)
I can guarantee you that such a driver is broken.
(Note that loop.c does look at BH_Dirty, but only for buffers it submitted
by itself)
> COMPLETELY IRRELEVANT, has no value here/now, and would
> not help us AT ALL until we start using 2.5.
>
> Why are you mentioning 2.5 at all?
Please excuse me for mentioning a kernel you don't actually use.
Please tell me the exact 2.4 patchlevel you use so that I'll
never have to bother you with kernel irrelevant to you..
> Why don't you comment on the other things I posted:
> (which I consider to be the real issues)
> - the pg locking clearly pulled in one place,
> - coalescing pagesync_t into pg with a single atomic var,
> - the if goto fiasco,
> - having extra code with one purpose: debugging,
> (not talking about BUG()) removed.
> - a centralized generic_make_request() calling scheme,
> rather than all over the place,
> - etc...
> This would make sense and help xfs improve, not nit-picking
> short conditionals.
What about submitting one patch per change, with a usefull comment
instead of ranting?
|