On Wed, 30 Oct 2002, Luben Tuikov wrote:
> Christoph Hellwig wrote:
> >
> > On Tue, Oct 29, 2002 at 07:43:35PM -0500, Luben Tuikov wrote:
> > > a ? a : b; <==> a ? : b;
FWIW, it generates a warning on my compiler. I wanted to see if it
generated any different assembly (I'm guessing it doesn't).
> > > 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!)
That's not what he said. If you want the VM to write a buffer, you set it
dirty. Then when it gets submitted, the dirty bit is cleared. As we do.
> 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.
I'm curious as to why a lower-level driver would ever need to look at the
state of the buffer. If the filesystem says "write this" shouldn't
the lower level driver just do it, and not second-guess things? To
me it makes sense that these flags should not affect lower level drivers.
If you look at __block_write_full_page, brw_kiovec, journal_commit_transaction,
submit_bh_for_writepage, and others, the kernel paradigm is to
-clear- the dirty bit and -set- uptodate before the buffer is submitted
to lower levels. I guess this is because those lower levels are expected
to do their job, i.e. write the thing to disk, so it will no longer be dirty,
and will now be uptodate.
> > 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*.
You also need to do things that are consistent with what everyone working
on the kernel will expect. If you feel that it is important to set Uptodate
only after you know that the buffer has been written, then you need to take
that crusade to LKML in general, I think.
> > Drivers should never ever look at BH_Dirty.
>
> (Can you guarantee that no such (stacking) drivers exist?)
Can you show me one that does? :) And is it generally considered correct?
> I see md does it, implicitly by turning it on -- I guess it's
> just being consistent.
I don't know enough about md to comment on why they do it this way.
> 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,
Hardly a fiasco, more of a coding style, perhaps it could be cleaned
up, if you have a patch that makes the code clearer, submit it.
> - having extra code with one purpose: debugging,
> (not talking about BUG()) removed.
What code is this? There is plenty of debug-specific code in xfs, if
some of it is compiled for non-debug kernels, please point this out.
Debugging code is there for a reason, though.
> - a centralized generic_make_request() calling scheme,
> rather than all over the place,
> - etc...
You'll find that we don't have much time to respond to "would be nice"
types of patches. As you've probably seen on the list, when someone
submits a patch that clearly fixes a bug, and can demonstrate that it is
correct, it gets accepted and applied.
Your "Page buf stuff" patch seems to do 4 things:
1) re-write a conditional
2) invert buffer head flag settings (discussed above)
3) make a new loop just for submitting the buffer heads, and
4) run tq_disk
1 and 3 seem to be no-ops, 2 seems to be incorrect (although I understand
that you disagree), and it's not clear why 4 is there, Steve feels that it
will adversely affect performance. If you can show otherwise, please do.
-Eric
|