Eric Sandeen wrote:
>
> 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.
I had been looking at md. Uptodate is off and dirty is on in the md code.
This behaviour also seems to make logical sense to me -- i.e. taking
a pessimistic view.
> 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.
Thanks for the mocking, but no, I don't have any particular feeling
regarding this, other than what I've mentioned makes sense to me (logically),
AND have seen it in the md code.
E.g. raid1.c, line 674-5 does:
mbh->b_state = (1<<BH_Req) | (1<<BH_Dirty) | (1<<BH_Mapped) | (1<<BH_Lock);
And I thought that it is for a reason; I guess they don't want to take
chances (I don't like taking changes either). But if you guys feel that
this is *incorrect*, so be it -- I don't care.
> Can you show me one that does? :) And is it generally considered correct?
No, but I don't want to take my chances, as I said.
``Generally considered correct''? -- I cannot speak for other ppl.
I saw it in md, and logically made sense to me (things I've read, seen, etc),
that's all.
> 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.
Idealistically speaking, (I'm one such), it does make a difference
for such small, insignificant patches to go in, so that after
10-20 of them, the code may be cleaner and have better design.
OTOH, I got involved with XFS just to submit and track bug 182,
since we've been getting it for many months, but before I had other
things to do (priorites). Then I got free and I was asked to take
a look at this and spent less than 2 days total looking at the thing.
With such little time on xfs, I'll never submit a serious patch.
And as you can see, the original post had one sentence in it:
``Maybe like this?'' -- it was never a serious patch, I never
intended it to be.
> > - 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.
Ok, I understand.
My mistake. My (idealistic) position is that kernel debuggers are not
very helpful and if code needs code to debug it, then the whole thing
needs rewriting.
> > - 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.
This wasn't one of those ``would be nice'', it was more of a
``maybe better and cleaner approach''. But, I don't care anymore.
XFS now works with lvm and md underneath it -- that's all
that matters now. Who cares what the code actually looks like?
> 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,
Sure. If you look, you'll see that the original posting had
``maybe like this?'' sentence at the begining (the only one).
OTOH, moving generic_make_request() out and a few more of those
``no-ops'' as you call them, will start to make for a cleaner and
more manageable code.
Didn't I get a reply, something of the sort:
``so many eyes have looked at this code ....''? -- My point exactly.
Think only that, BEFORE the patch from Asano, that same 3) would
have looked like a no-ops to you, but that's all that was needed
to get rid of bug 182. In fact, 3) or the patch from Asano.
> 2 seems to be incorrect (although I understand that you disagree),
No, I don't care. Has nothing to do with agreeing.
> and it's not clear why 4 is there, Steve feels that it
> will adversely affect performance. If you can show otherwise, please do.
The whole point was to have run_task_queue(&tq_disk) pulled out in _one_ place,
nicely and cleanly.
Does XFS now work with LVM and md? -- Yes. So, now I'll go back to the
other things I need and have to do.
Good luck to all of you,
--
Luben
|