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. That is just my personal opinion, will fix it. Thanks, Gao Xiang > Thanks, >
