> The code is much simpler and much more straightforward, and it makes
> sense. No "tricks" involved.
>
> See it my way:
>
> - I have simpler code, with one less made-up bit of information. If you
> look at the patches, you'll notice that NOTHING actually ever needs to
> test the above combination - it just falls out as the natural
> behaviour.
only because holes are not really optimized for currently. With the
buffer_hole() logic i could add 'delayed clearing' of holes as a natural
extension. The current code is full of traps and _wrong_ logic, eg. the
partial write case now does:
if (!buffer_mapped(bh)) {
err = inode->i_op->get_block(inode, block, bh, 1);
if (err)
goto out;
}
if (!buffer_uptodate(bh) && (start_offset ||
(end_bytes && (i == end_block)))) {
if (buffer_new(bh)) {
memset(bh->b_data, 0, bh->b_size);
} else {
lock_kernel();
ll_rw_block(READ, 1, &bh);
this will obviously fail if holes are a little bit different and are not
uptodate anymore.
if you do not want those optimization then fine, but i think fs-holes
_are_ useful. They are the same thing to filesystems as the initial
clear_page() is to newly mapped in anonymous pages.
the fact is, it does not 'fall out automatically', it falls out
_accidentally_ in the read_full_page case. And it does _not_ fall out
nicely in other cases. (!mapped, uptodate, !dirty) does _not_ express
holes in a straightforward way IMO. It just happens to accidentally
coincide with some currently invalid caching state bits in the only place
where it's being used right now. It happens to be in the !mapped branch
because holes are currently cleared synchronously, thus they are always
cleared when first mapped. They happen to be in the '!mapped' branch after
the block is allocated only because the lowlevel fs sets it explicitly:
phys = ext2_block_map(inode, iblock);
if (phys) {
bh_result->b_dev = inode->i_dev;
bh_result->b_blocknr = phys;
bh_result->b_state |= (1UL << BH_Mapped);
}
In the write case it's everything but not logical.
> - more complex code, with more state, that doesn't give anything new and
> adds more rules that have to be tested for.
> So obviously I'll drop the BH_Hole bit - it isn't buying us anything at
> all, and it _is_ setting us up for coherency problems.
i agree about this if they are not optimized for and are just a secondary
feature - i wanted to change this.
> > !Mapped && Uptodate && !Dirty can be a valid combination
> > in the future as well - we might want to destroy buffer-heads (the
> > mapping) under a data block for legitimate reasons, eg. in the future it
> > might be possible to move the device under a filesystem, or we simply have
> > a filesystem with nonpermanent mappings, eg. a filesystem might choose to
> > destroy the mapping after the IO has been finished.
>
> I'll believe you when I see it. My motto is: never overdesign. It sounds
> like you're making up reasons to overdesign something that isn't needed.
no, i simply have some optimizations planned which will be much uglier
with 'if (!mapped && uptodate && !dirty)' than with a 'if (hole)'. And not
because i cannot write macros to wrap the first one, but because the first
one is embedded into the current code in sometimes subtle ways. I
definitely feel uneasy about building false and misleading logic into the
generic block handling code.
buffer_mapped() now means 'this block has a non-hole mapping to the fs'
buffer_mapped() should mean 'this block has a mapping to the fs'
hm, i think the real source of disagreement is that you are thinking about
holes as 'not yet allocated filesystem blocks which have to be security
pre-cleared if read', and i think they are 'special, _mapped_ data-blocks
which represent a common type of data'.
anyway, this is not that important, it's clearly a secondary issue to all
those other cleanups that were done, i just wanted to explain my thinking
wrt. BH_Hole.
-- mingo
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.rutgers.edu
Please read the FAQ at http://www.tux.org/lkml/