Evgeniy Polyakov wrote:
> Hi.
>
> On Mon, Feb 09, 2009 at 03:24:13PM +0200, Boaz Harrosh (bharrosh@xxxxxxxxxxx) wrote:
>
>> +void exofs_set_link(struct inode *dir, struct exofs_dir_entry *de,
>> + struct page *page, struct inode *inode)
>> +{
>> + loff_t pos = page_offset(page) +
>> + (char *) de - (char *) page_address(page);
>> + unsigned len = le16_to_cpu(de->rec_len);
>> + int err;
>> +
>> + lock_page(page);
>> + err = exofs_write_begin(NULL, page->mapping, pos, len,
>> + AOP_FLAG_UNINTERRUPTIBLE, &page, NULL);
>> + BUG_ON(err);
>
> How unfriendly :)
> simple_write_begin() may fail if there is no memory or appropriate
> cgroup does not allow to charge more memory.
>
You are right on the money. I'll go and revisit all the BUGs and BUG_ONs
Thanks good catch.
>> + unlock_page(page);
>> + exofs_put_page(page);
>> + }
>> + BUG();
>> + return -EINVAL;
>> +
>
> So it will crash the system if directory entry does not contain any
> data? What was wrong with -EINVAL?
>
Yes, thanks, will fix
> Also, dir_pages(), readpage_done() and similar functions scream for less
> generic names, and at least dir_pages() is already implemented in another
> 5 filesystems.
>
I will fix that too. I thought I changed all these, I must have missed
a few.
We are so used to filesystems been loadable modules we never try to compile
a few in-kernel.
>> +int exofs_delete_entry(struct exofs_dir_entry *dir, struct page *page)
>> +{
>> + struct address_space *mapping = page->mapping;
>> + struct inode *inode = mapping->host;
>> + struct exofs_sb_info *sbi = inode->i_sb->s_fs_info;
>> + char *kaddr = page_address(page);
>> + unsigned from = ((char *)dir - kaddr) & ~(exofs_chunk_size(inode)-1);
>> + unsigned to = ((char *)dir - kaddr) + le16_to_cpu(dir->rec_len);
>> + loff_t pos;
>> + struct exofs_dir_entry *pde = NULL;
>> + struct exofs_dir_entry *de = (struct exofs_dir_entry *) (kaddr + from);
>> + int err;
>> +
>> + while ((char *)de < (char *)dir) {
>
> They have the same type, why is it needed to cast them to char pointer?
>
Will fix, thanks
>> + if (de->rec_len == 0) {
>> + EXOFS_ERR("ERROR: exofs_delete_entry:"
>> + "zero-length directory entry");
>> + err = -EIO;
>> + goto out;
>> + }
>> + pde = de;
>> + de = exofs_next_entry(de);
>> + }
>> + if (pde)
>> + from = (char *)pde - (char *)page_address(page);
>> + pos = page_offset(page) + from;
>> + lock_page(page);
>> + err = exofs_write_begin(NULL, page->mapping, pos, to - from, 0,
>> + &page, NULL);
>> + BUG_ON(err);
>
> Ugh, in the exofs_make_empty() it is handled without so visible
> pain.
>
Yep, will fix
>> + if (pde)
>> + pde->rec_len = cpu_to_le16(to - from);
>> + dir->inode_no = 0;
>> + err = exofs_commit_chunk(page, pos, to - from);
>> + inode->i_ctime = inode->i_mtime = CURRENT_TIME;
>> + mark_inode_dirty(inode);
>> + sbi->s_numfiles--;
>> +out:
>> + exofs_put_page(page);
>> + return err;
>> +}
<snip>
>> +
>> + atomic_inc(&inode->i_count);
>> +
>> + ret = exofs_async_op(or, create_done, inode, oi->i_cred);
>> + if (ret) {
>> + atomic_dec(&inode->i_count);
>
> igrab()/iput()?
>
Thanks, makes much more sense. Sorry leftovers from 2.6.10
>> + osd_end_request(or);
>> + return ERR_PTR(-EIO);
>> + }
>> + atomic_inc(&sbi->s_curr_pending);
>> +
>> + return inode;
>> +}
>
>> +static int exofs_mkdir(struct inode *dir, struct dentry *dentry, int mode)
>> +{
>> + struct inode *inode;
>> + int err = -EMLINK;
>> +
>> + if (dir->i_nlink >= EXOFS_LINK_MAX)
>> + goto out;
>> +
>> + inode_inc_link_count(dir);
>> +
>> + inode = exofs_new_inode(dir, S_IFDIR | mode);
>> + err = PTR_ERR(inode);
>> + if (IS_ERR(inode))
>> + goto out_dir;
>> +
>> + inode->i_op = &exofs_dir_inode_operations;
>> + inode->i_fop = &exofs_dir_operations;
>> + inode->i_mapping->a_ops = &exofs_aops;
>> +
>> + inode_inc_link_count(inode);
>> +
>> + err = exofs_make_empty(inode, dir);
>> + if (err)
>> + goto out_fail;
>> +
>> + err = exofs_add_link(dentry, inode);
>> + if (err)
>> + goto out_fail;
>> +
>> + d_instantiate(dentry, inode);
>> +out:
>> + return err;
>> +
>> +out_fail:
>> + inode_dec_link_count(inode);
>> + inode_dec_link_count(inode);
>
> Why two decrements, will it be ok after exofs_make_empty() fail when it
> was incremented only once?
>
That's hard to say, I'll investigate it some more.
Thanks
>> + iput(inode);
>> +out_dir:
>> + inode_dec_link_count(dir);
>> + goto out;
>> +}
>
Most valuable input, thank you for taking the time to review.
Boaz
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
