[lkml]   [2000]   [Dec]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [CFT][RFC] ext2_new_inode() fixes and cleanup

On Fri, 1 Dec 2000, Andreas Dilger wrote:

> Alexander, Ted,
> I was taking a hard look at the proposed changes. In load_inode_bitmap()
> we shouldn't be cacheing the I/O error case (this was in the old code too).

I know. I left it in place since I don't like the idea of putting many
not-absolutely-trivial changes into one chunk...

> We should probably reconcile the changes here with load_block_bitmap(),
> or for that matter, we should make a single function and pass in a
> pointer to s_{block,inode}_bitmap and s_{block,inode}_bitmap_number
> arrays and save the duplicate code maintenance (even if inline).

The former - definitely yes (I just don't want to deal with changes in
too many places at once), the latter... I'm not sure that it's a right
thing. Generic manipulations of LRU - sure, but let's make it a helper
function and lets take the actual loading of the bitmaps into its
(2) callers.

> If we have a generic function like so:
> struct buffer_head *load_bitmap(struct super_block *sb,
> unsigned int block_group,
> unsigned long *bitmap_number,
> struct buffer_head *bitmap,
> size_t gdt_offset)

Too many arguments. Exposed to all callers...

BTW, I would actually start with initializing the caches with NULLs and
forgetting about the counters (->s_loaded_foo_bitmap). That would allow
to shrink the load_foo_bitmap quite seriously - look at the code and
you'll see. Then code duplication becomes almost a non-issue.

> Comments/changes are enclosed below, with sh style # comments by me.
> PS - discussion on this should probably move to ext2-devel or fsdevel.

> > + if (sbi->s_inode_bitmap_number[slot] == slot)
> > + goto found;
> /* FIXME: when online shrinking we may go from case B to A */
Keeps the behaviour of old code.

> > + ext2_panic (sb, "load_inode_bitmap",
> > + "block_group != inode_bitmap_number");

> # Need to fix callers of load_inode_bitmap() to check for NULL return

If you do the return NULL instead of return ERR_PTR(...). OTOH, right
now we only do ERR_PTR(-EIO), so we might pass NULL just fine. I'm
not sure that we'll never want other error values, though...

> # In find_cg_other() we should probably should also check for free blocks,
> # to give inode a chance to be close to any blocks it will likely soon
> # allocate. Don't need to byte swap to test non-zeroness... It would be
> # nice to use "gdt" instead of "cg", to keep it consistent with the rest
> # of the ext2 code... Maybe find_group_dir() and find_group_other()?

<shrug> History. It used to be called "cylinder group" in FFS.

> # In ext2_new_inode() you removed the case where the bit was set between
> # time we searched and the time we set it. This probably can't happen
> # while we hold the superblock lock. It may be worth keeping for when we
> # go to per-group locks.

No, it isn't. Notice that find_cg_foo() updates the per-group counters.
So nobody can take the last free bit from us.

> /* With the superblock lock we are safe here, but not once it's gone.
> if (ext2_set_bit(j, bh->b_data)) {
> ext2_warning(sb, __FUNCTION__,
> "bit already set for inode %d", j);
> goto repeat;
> }

No. Sorry, but "goto repeat" is a very loud "we have a race here" alarm.
Doing that after you've found and fixed recoverable error is OK. Using
that to deal with races is a recipe for big trouble. Protect the
search-and-set with a spinlock and that's it. In any case, if we do the
"goto repeat" variant we should not do ext2_warning() (it _is_ bogus)
and we should return only to ext2_find_first_zero_bit() - we had already
reserved an empty inode in the counter and repeating the whole search
for appropriate group is a huge overkill.

> */
> # Need to check that we got a good gdp back here...
> > +fail2:
> > + gdp = ext2_get_group_desc (sb, i, &bh2);

Why? ext2_get_group_desc() can't fail, since we've already called it with
that argument and ->s_group_desc[] is not cleaned. If you are thinking about
resize - let's add a resize_sem and make current users of lock_super() do
down_read() and resizer do down_write() - no additional contention for normal
case and you really don't want to do resize while allocating. Notice that
as soon as we start removing elements from ->s_group_desc[] we are in for
a major PITA - brelse() on a buffer_head * that can be in use right now...
Not nice.

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to
Please read the FAQ at

 \ /
  Last update: 2005-03-22 12:49    [W:0.047 / U:0.388 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site