Christoph Hellwig wrote:
>
> On Tue, Oct 29, 2002 at 07:43:35PM -0500, Luben Tuikov wrote:
> > a ? a : b; <==> a ? : b;
>
> It's a non-standard extension, and IMHO not a very good idea.
> If your expression is long enough that you'd need it you
> should be using explicit if-statements anyway.
I, OTOH, do NOT have any particular problems with *new* things.
You see it once, you see it twice and eventually get used to it
and use it yourself. (Like struct list_head, which I really love!)
I do think that it's a good idea.
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?
My patch will *never* make it to the xfs source so why do you care?
It was just an example. Next time I'll know and save 1/2 hour of
my precious time on this planet.
> > I consider the md code to be a correct implementation. Please
> > see the code which I refered to in the original message.
>
> 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.
> > If the buffer should be written out, the dirty bit should be turned on.
> > This makes sense anywhich way you look at it and think about it.
> > This makes sense from 1. the code I've seen in the kernel (md)
> > and from 2. logical point of view and from 3. my old OS course textbook
> > (``the dinosaur book'').
>
> 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.
> 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.
Logically Uptodate is false, before the buffer has been written out
to the media (for WRITE operations). And I try to only do things
which *make sense*.
Think about it: if the buffer was never written to the media,
but traveled to the lower driver: Uptodate=false (set by the callback),
now what is the difference wrt to the buffer if it didn't travel to
the lower driver? Uptodate should be set to false, Dirty to true,
then submit -- makes sense?
> Drivers should never ever look at BH_Dirty.
(Can you guarantee that no such (stacking) drivers exist?)
I see md does it, implicitly by turning it on -- I guess it's
just being consistent.
Rather than nit-picking on my emails, why don't you just tell
me what would MAKE SENSE?
Would it *make sense* to turn on BH_Dirty before submitting to
the lower driver?
You could've easily said: Yes, it makes sense to turn it on
before submitting a bh for write.
> Forstunately the I/O
> containers and buffer_heads are completly separate in 2.5 to avoid
> such mess.
I repeat again: we're using 2.4.x.
I also know about 2.5, bios, blah, blah, but this is
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?
I cannot possibly believe (by your attitude towards my posts
here) that you think any improvement in xfs is due. All I've
seen is you nit-picking on my posts.
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.
Good luck guys,
--
Luben
|