On 2018/8/10 14:33, Chao Yu wrote:
> Hi Xiang,
>
> On 2018/8/10 14:15, Gao Xiang wrote:
>> Hi Chao,
>>
>> On 2018/8/10 14:08, Chao Yu wrote:
>>> Hi Xiang,
>>>
>>> On 2018/8/10 11:47, Gao Xiang wrote:
>>>> Hi Chao,
>>>>
>>>> On 2018/8/10 11:40, Chao Yu wrote:
>>>>> On 2018/8/1 14:01, Gao Xiang wrote:
>>>>>> This patch enhances the missing error handling code for
>>>>>> xattr submodule, which improves the stability for the rare cases.
>>>>>>
>>>>>> Signed-off-by: Gao Xiang <[email protected]>
>>>>>> ---
>>>>>> drivers/staging/erofs/xattr.c | 114
>>>>>> ++++++++++++++++++++++++++++--------------
>>>>>> 1 file changed, 77 insertions(+), 37 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/staging/erofs/xattr.c
>>>>>> b/drivers/staging/erofs/xattr.c
>>>>>> index 702434b..bd911bb 100644
>>>>>> --- a/drivers/staging/erofs/xattr.c
>>>>>> +++ b/drivers/staging/erofs/xattr.c
>>>>>> @@ -24,16 +24,25 @@ struct xattr_iter {
>>>>>>
>>>>>> static inline void xattr_iter_end(struct xattr_iter *it, bool atomic)
>>>>>> {
>>>>>> - /* only init_inode_xattrs use non-atomic once */
>>>>>> + /* the only user of kunmap() is 'init_inode_xattrs' */
>>>>>> if (unlikely(!atomic))
>>>>>> kunmap(it->page);
>>>>>> else
>>>>>> kunmap_atomic(it->kaddr);
>>>>>> +
>>>>>> unlock_page(it->page);
>>>>>> put_page(it->page);
>>>>>> }
>>>>>>
>>>>>> -static void init_inode_xattrs(struct inode *inode)
>>>>>> +static inline void xattr_iter_end_final(struct xattr_iter *it)
>>>>>> +{
>>>>>> + if (unlikely(it->page == NULL))
>>>>>> + return;
>>>>>> +
>>>>>> + xattr_iter_end(it, true);
>>>>>> +}
>>>>>> +
>>>>>> +static int init_inode_xattrs(struct inode *inode)
>>>>>> {
>>>>>> struct xattr_iter it;
>>>>>> unsigned i;
>>>>>> @@ -44,7 +53,7 @@ static void init_inode_xattrs(struct inode *inode)
>>>>>> bool atomic_map;
>>>>>>
>>>>>> if (likely(inode_has_inited_xattr(inode)))
>>>>>> - return;
>>>>>> + return 0;
>>>>>>
>>>>>> vi = EROFS_V(inode);
>>>>>> BUG_ON(!vi->xattr_isize);
>>>>>> @@ -55,7 +64,8 @@ static void init_inode_xattrs(struct inode *inode)
>>>>>> it.ofs = erofs_blkoff(iloc(sbi, vi->nid) + vi->inode_isize);
>>>>>>
>>>>>> it.page = erofs_get_inline_page_nofail(inode, it.blkaddr);
>>>>> Need to introduce and use erofs_get_inline_page()?
>>>>>
>>>>>> - BUG_ON(IS_ERR(it.page));
>>>>>> + if (unlikely(IS_ERR(it.page)))
>>>>>> + return PTR_ERR(it.page);
>>>>>>
>>>>>> /* read in shared xattr array (non-atomic, see kmalloc below) */
>>>>>> it.kaddr = kmap(it.page);
>>>>>> @@ -64,9 +74,12 @@ static void init_inode_xattrs(struct inode *inode)
>>>>>> ih = (struct erofs_xattr_ibody_header *)(it.kaddr + it.ofs);
>>>>>>
>>>>>> vi->xattr_shared_count = ih->h_shared_count;
>>>>>> - vi->xattr_shared_xattrs = (unsigned *)kmalloc_array(
>>>>>> - vi->xattr_shared_count, sizeof(unsigned),
>>>>>> - GFP_KERNEL | __GFP_NOFAIL);
>>>>>> + vi->xattr_shared_xattrs = kmalloc_array(vi->xattr_shared_count,
>>>>>> + sizeof(unsigned),
>>>>>> GFP_KERNEL);
>>>>>> + if (unlikely(vi->xattr_shared_xattrs == NULL)) {
>>>>>> + xattr_iter_end(&it, atomic_map);
>>>>>> + return -ENOMEM;
>>>>>> + }
>>>>>>
>>>>>> /* let's skip ibody header */
>>>>>> it.ofs += sizeof(struct erofs_xattr_ibody_header);
>>>>>> @@ -79,7 +92,8 @@ static void init_inode_xattrs(struct inode *inode)
>>>>>>
>>>>>> it.page = erofs_get_meta_page_nofail(sb,
>>>>>> ++it.blkaddr, S_ISDIR(inode->i_mode));
>>>>> Need to use erofs_get_meta_page() instead?
>>>>>
>>>>> Thanks,
>>>> My consideration is for example,
>>>>
>>>> init_inode_xattrs itself can be seen as a part of inode initialization
>>>> (other fs does it in
>>>> inode initialization, but erofs does it in the first xattr request), if it
>>>> fail, the only way
>>>> is to retry again...but with more overhead to retry...
>>> I think we can easily return an error to user no matter we encounter
>>> -ENOMEM or
>>> -EIO, so erofs_get_meta_page() is enough here, thoughts?
>>>
>> OK, it depends on users (kernel / programs). I think they will simply retry
>> again in the outer loop,
>> which will cause more overhead therefore for inode metadata (not xattrs), I
>> personally tend to retry inside.
> Yes, both are okay, as there is no such restriction in manual or other
> documents
> saying syscall should success any time, so I think let's take easy to let user
> to handle this condition.
>
> And one more concern is, for extremely environment, like with very few memory,
> retrying in 'nofail' function, make syscall looks like hanging in kernel side,
> result in apps can not respond to user, that's not a good experience for user.
>
> Also like f2fs, nofail version function is abused, I think it needs to be
> limited and it's better to be rectified like other filesystem.
>
Agreed. Thanks for your explainations. :)
Thanks,
Gao Xiang
> Thanks,
>
