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

Reply via email to