xfs
[Top] [All Lists]

Re: [RFC PATCH] xfs: enables file data inlining in inodes

To: xfs@xxxxxxxxxxx
Subject: Re: [RFC PATCH] xfs: enables file data inlining in inodes
From: Brian Foster <bfoster@xxxxxxxxxx>
Date: Tue, 16 Oct 2012 11:18:32 -0400
In-reply-to: <20121016144829.GA8752@xxxxxxxxxxxxxxxxxxxxxxxxxxxx>
References: <1349985158-9952-1-git-send-email-cmaiolino@xxxxxxxxxx> <507837E6.4040807@xxxxxxxxxx> <20121015171904.GB8376@xxxxxxxxxxxxxxxxxxxxxxxxxxxx> <507C5648.3020306@xxxxxxxxxx> <20121016144829.GA8752@xxxxxxxxxxxxxxxxxxxxxxxxxxxx>
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/20120911 Thunderbird/15.0.1
On 10/16/2012 10:48 AM, Carlos Maiolino wrote:
> On Mon, Oct 15, 2012 at 02:30:32PM -0400, Brian Foster wrote:
>> On 10/15/2012 01:19 PM, Carlos Maiolino wrote:
>>> On Fri, Oct 12, 2012 at 11:31:50AM -0400, Brian Foster wrote:
>>>> On 10/11/2012 03:52 PM, Carlos Maiolino wrote:
>>>>> Hi,
>>>>>
...
>> I'm not quite sure I follow what you mean by catching file changes, so I
>> certainly could be missing something... 
> 
> An example: If you extend a file, you need to check if the file still fits in
> the inode. I'm not sure that only i_size_read(inode) will check for this, 
> that's
> why I added end_offset + i_size_read(), but, I believe it's wrong (end_offset 
> +
> i_size_read).
> 

FWIW, there is an i_size_write() in generic_write_end() (via
xfs_vm_write_end()) that looks like it handles the file extension case.

>> but it seems like the code
>> earlier in xfs_vm_writepage() already handles the truncate case. If the
>> file size increased in that time, would this code really care? Wouldn't
>> that mean you'd have to convert the file at that point, presumably when
>> you get a writepage on one of the pages that falls outside the available
>> space?
>>
>> Perhaps this gets more into detection of the current inode format when
>> you have to convert back and forth and this logic naturally becomes more
>> complicated (and thus I'm just reading too far ahead :P).
>>
> Yes, as the patch description says, it doesn't care about file conversion yet 
> :)
> this only cares about write/read new files into inodes, I want to make sure 
> I'm
> doing this part properly, before move on the the next step, which is take care
> of these conversions, if I take care of everything at the same time I'll get
> crazy and have nothing done at the same time :-)
> 

I hear you. ;)

Brian

>> Brian
>>
>>>
>>>> Brian
>>>>
>>>>> +         err = xfs_inline_write(ip, page, bh, end_offset);
>>>>> +
>>>>> +         if (err)
>>>>> +                 goto error;
>>>>> +         return 0;
>>>>> + }
>>>>> +
>>>>>   offset = page_offset(page);
>>>>>   type = XFS_IO_OVERWRITE;
>>>>>  
>>>>> @@ -1624,20 +1723,43 @@ xfs_vm_bmap(
>>>>>  
>>>>>  STATIC int
>>>>>  xfs_vm_readpage(
>>>>> - struct file             *unused,
>>>>> + struct file             *filp,
>>>>>   struct page             *page)
>>>>>  {
>>>>> - return mpage_readpage(page, xfs_get_blocks);
>>>>> + struct inode            *inode = filp->f_mapping->host;
>>>>> + struct xfs_inode        *ip = XFS_I(inode);
>>>>> +
>>>>> + if (ip->i_d.di_format == XFS_DINODE_FMT_LOCAL)
>>>>> +         return xfs_inline_read(ip, page, page->mapping);
>>>>> + else
>>>>> +         return mpage_readpage(page, xfs_get_blocks);
>>>>>  }
>>>>>  
>>>>>  STATIC int
>>>>>  xfs_vm_readpages(
>>>>> - struct file             *unused,
>>>>> + struct file             *filp,
>>>>>   struct address_space    *mapping,
>>>>>   struct list_head        *pages,
>>>>>   unsigned                nr_pages)
>>>>>  {
>>>>> - return mpage_readpages(mapping, pages, nr_pages, xfs_get_blocks);
>>>>> + struct inode            *inode = filp->f_mapping->host;
>>>>> + struct xfs_inode        *ip = XFS_I(inode);
>>>>> +
>>>>> + if (ip->i_d.di_format == XFS_DINODE_FMT_LOCAL) {
>>>>> +         struct page     *page = list_first_entry(pages, struct page, 
>>>>> lru);
>>>>> +
>>>>> +         ASSERT(i_size_read(VFS_I(ip) <= PAGE_CACHE_SIZE));
>>>>> +
>>>>> +         list_del(&page->lru);
>>>>> +         if(!(add_to_page_cache_lru(page, mapping,
>>>>> +                                     page->index, GFP_KERNEL)))
>>>>> +                 return xfs_inline_read(ip, page, page->mapping);
>>>>> +
>>>>> +         page_cache_release(page);
>>>>> +         return 0;
>>>>> + } else {
>>>>> +         return mpage_readpages(mapping, pages, nr_pages, 
>>>>> xfs_get_blocks);
>>>>> + }
>>>>>  }
>>>>>  
>>>>>  const struct address_space_operations xfs_address_space_operations = {
>>>>> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
>>>>> index 2778258..5e56e5c 100644
>>>>> --- a/fs/xfs/xfs_inode.c
>>>>> +++ b/fs/xfs/xfs_inode.c
>>>>> @@ -287,18 +287,6 @@ xfs_iformat(
>>>>>   case S_IFDIR:
>>>>>           switch (dip->di_format) {
>>>>>           case XFS_DINODE_FMT_LOCAL:
>>>>> -                 /*
>>>>> -                  * no local regular files yet
>>>>> -                  */
>>>>> -                 if (unlikely(S_ISREG(be16_to_cpu(dip->di_mode)))) {
>>>>> -                         xfs_warn(ip->i_mount,
>>>>> -                 "corrupt inode %Lu (local format for regular file).",
>>>>> -                                 (unsigned long long) ip->i_ino);
>>>>> -                         XFS_CORRUPTION_ERROR("xfs_iformat(4)",
>>>>> -                                              XFS_ERRLEVEL_LOW,
>>>>> -                                              ip->i_mount, dip);
>>>>> -                         return XFS_ERROR(EFSCORRUPTED);
>>>>> -                 }
>>>>>  
>>>>>                   di_size = be64_to_cpu(dip->di_size);
>>>>>                   if (unlikely(di_size > XFS_DFORK_DSIZE(dip, 
>>>>> ip->i_mount))) {
>>>>> @@ -2471,7 +2459,8 @@ xfs_iflush_int(
>>>>>   if (S_ISREG(ip->i_d.di_mode)) {
>>>>>           if (XFS_TEST_ERROR(
>>>>>               (ip->i_d.di_format != XFS_DINODE_FMT_EXTENTS) &&
>>>>> -             (ip->i_d.di_format != XFS_DINODE_FMT_BTREE),
>>>>> +             (ip->i_d.di_format != XFS_DINODE_FMT_BTREE) &&
>>>>> +             (ip->i_d.di_format != XFS_DINODE_FMT_LOCAL),
>>>>>               mp, XFS_ERRTAG_IFLUSH_3, XFS_RANDOM_IFLUSH_3)) {
>>>>>                   xfs_alert_tag(mp, XFS_PTAG_IFLUSH,
>>>>>                           "%s: Bad regular inode %Lu, ptr 0x%p",
>>>>>
>>>>
>>>> _______________________________________________
>>>> xfs mailing list
>>>> xfs@xxxxxxxxxxx
>>>> http://oss.sgi.com/mailman/listinfo/xfs
>>>
>>
>> _______________________________________________
>> xfs mailing list
>> xfs@xxxxxxxxxxx
>> http://oss.sgi.com/mailman/listinfo/xfs
> 

<Prev in Thread] Current Thread [Next in Thread>