On Wed, 2007-07-11 at 10:34 -0700, Andrew Morton wrote:
> On Wed, 11 Jul 2007 06:10:56 -0600 Andreas Dilger <[EMAIL PROTECTED]> wrote:
>
> > On Jul 10, 2007 16:32 -0700, Andrew Morton wrote:
> > > > err = ext4_reserve_inode_write(handle, inode, &iloc);
> > > > + if (EXT4_I(inode)->i_extra_isize <
> > > > + EXT4_SB(inode->i_sb)->s_want_extra_isize &&
> > > > + !(EXT4_I(inode)->i_state & EXT4_STATE_NO_EXPAND)) {
> > > > + /* We need extra buffer credits since we may write into
> > > > EA block
> > > > + * with this same handle */
> > > > + if ((jbd2_journal_extend(handle,
> > > > + EXT4_DATA_TRANS_BLOCKS(inode->i_sb))) ==
> > > > 0) {
> > > > + ret = ext4_expand_extra_isize(inode,
> > > > +
> > > > EXT4_SB(inode->i_sb)->s_want_extra_isize,
> > > > + iloc, handle);
> > > > + if (ret) {
> > > > + EXT4_I(inode)->i_state |=
> > > > EXT4_STATE_NO_EXPAND;
> > > > + if (!expand_message) {
> > > > + ext4_warning(inode->i_sb,
> > > > __FUNCTION__,
> > > > + "Unable to expand inode %lu.
> > > > Delete"
> > > > + " some EAs or run e2fsck.",
> > > > + inode->i_ino);
> > > > + expand_message = 1;
> > > > + }
> > > > + }
> > > > + }
> > > > + }
> > >
> > > Maybe that message should come out once per mount rather than once per
> > > boot
> > > (or once per modprobe)?
> >
> > Probably true.
> >
> > > What are the consequences of a jbd2_journal_extend() failure in here?
> >
> > Not fatal, just like every user of i_extra_isize. If the inode isn't a
> > large inode, or it can't be expanded then there will be a minor loss of
> > functionality on that inode. If the i_extra_isize is critical, then
> > the sysadmin will have run e2fsck to force s_min_extra_isize large enough.
> >
> > Note that this is only applicable for filesystems which are upgraded. For
> > new inodes (i.e. all inodes that exist in the filesystem if it was always
> > run on a kernel with the currently understood extra fields) then this will
> > never be invoked (until such a time new extra fields are added).
>
> I'd suggest that we get a comment in the code explaining this: this
> unchecked error does rather stand out.
>
> > > > + if (EXT4_I(inode)->i_file_acl) {
> > > > + bh = sb_bread(inode->i_sb, EXT4_I(inode)->i_file_acl);
> > > > + error = -EIO;
> > > > + if (!bh)
> > > > + goto cleanup;
> > > > + if (ext4_xattr_check_block(bh)) {
> > > > + ext4_error(inode->i_sb, __FUNCTION__,
> > > > + "inode %lu: bad block %llu",
> > > > inode->i_ino,
> > > > + EXT4_I(inode)->i_file_acl);
> > > > + error = -EIO;
> > > > + goto cleanup;
> > > > + }
> > > > + base = BHDR(bh);
> > > > + first = BFIRST(bh);
> > > > + end = bh->b_data + bh->b_size;
> > > > + min_offs = end - base;
> > > > + free = ext4_xattr_free_space(first, &min_offs, base,
> > > > + &total_blk);
> > > > + if (free < new_extra_isize) {
> > > > + if (!tried_min_extra_isize &&
> > > > s_min_extra_isize) {
> > > > + tried_min_extra_isize++;
> > > > + new_extra_isize = s_min_extra_isize;
> > >
> > > Aren't we missing a brelse(bh) here?
> >
> > Seems likely, yes.
>
> OK - could we get a positive ack from someone indicating that this will get
> looked at? Because I am about to forget about it.
I will send a patch to add the comments and make the suggested
corrections.
Thanks,
Kalpak.
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to [EMAIL PROTECTED]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
-
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
