lkml.org 
[lkml]   [2007]   [May]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 4/5] ext4: fallocate support in ext4
    On May 03, 2007  21:31 -0700, Andrew Morton wrote:
    > On Thu, 26 Apr 2007 23:43:32 +0530 "Amit K. Arora" <aarora@linux.vnet.ibm.com> wrote:
    > > + * ext4_fallocate:
    > > + * preallocate space for a file
    > > + * mode is for future use, e.g. for unallocating preallocated blocks etc.
    > > + */
    >
    > This description is rather thin. What is the filesystem's actual behaviour
    > here? If the file is using extents then the implementation will do
    > <something>. If the file is using bitmaps then we will do <something else>.
    >
    > But what? Here is where it should be described.

    My understanding is that glibc will handle zero-filling of files for
    filesystems that do not support fallocate().

    > > +int ext4_fallocate(struct inode *inode, int mode, loff_t offset, loff_t len)
    > > +{
    > > + handle_t *handle;
    > > + ext4_fsblk_t block, max_blocks;
    > > + int ret, ret2, nblocks = 0, retries = 0;
    > > + struct buffer_head map_bh;
    > > + unsigned int credits, blkbits = inode->i_blkbits;
    > > +
    > > + /* Currently supporting (pre)allocate mode _only_ */
    > > + if (mode != FA_ALLOCATE)
    > > + return -EOPNOTSUPP;
    > > +
    > > + if (!(EXT4_I(inode)->i_flags & EXT4_EXTENTS_FL))
    > > + return -ENOTTY;
    >
    > So we don't implement fallocate on bitmap-based files! Well that's huge
    > news. The changelog would be an appropriate place to communicate this,
    > along with reasons why, or a description of the plan to fix it.
    >
    > Also, posix says nothing about fallocate() returning ENOTTY.

    I _think_ this is to convince glibc to do the zero-filling in userspace,
    but I'm not up on the API specifics.

    > > + block = offset >> blkbits;
    > > + max_blocks = (EXT4_BLOCK_ALIGN(len + offset, blkbits) >> blkbits)
    > > + - block;
    > > + mutex_lock(&EXT4_I(inode)->truncate_mutex);
    > > + credits = ext4_ext_calc_credits_for_insert(inode, NULL);
    > > + mutex_unlock(&EXT4_I(inode)->truncate_mutex);
    >
    > Now I'm mystified. Given that we're allocating an arbitrary amount of disk
    > space, and that this disk space will require an arbitrary amount of
    > metadata, how can we work out how much journal space we'll be needing
    > without at least looking at `len'?

    Good question.

    The uninitialized extent can cover up to 128MB with a single entry.
    If @path isn't specified, then ext4_ext_calc_credits_for_insert()
    function returns the maximum number of extents needed to insert a leaf,
    including splitting all of the index blocks. That would allow up to 43GB
    (340 extents/block * 128MB) to be preallocated, but it still needs to take
    the size of the preallocation into account (adding 3 blocks per 43GB - a
    leaf block, a bitmap block and a group descriptor).

    Also, since @path is not being given then truncate_mutex is not needed.

    > > + ret = ext4_ext_get_blocks(handle, inode, block,
    > > + max_blocks, &map_bh,
    > > + EXT4_CREATE_UNINITIALIZED_EXT, 0);
    > > + BUG_ON(!ret);
    >
    > BUG_ON is vicious. Is it really justified here? Possibly a WARN_ON and
    > ext4_error() would be safer and more useful here.

    Ouch, not very friendly error handling.

    Cheers, Andreas
    --
    Andreas Dilger
    Principal Software Engineer
    Cluster File Systems, Inc.

    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2007-05-07 21:53    [W:0.024 / U:31.908 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site