Re: [PATCH 5/8] exofs: dir_inode and directory operations

[Boaz Harrosh <bharrosh@panasas.com>]

Evgeniy Polyakov wrote:
> Hi.
> 
> On Mon, Feb 09, 2009 at 03:24:13PM +0200, Boaz Harrosh (bharrosh@panasas.com) 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