Re: [RFC] persistent store (version 3) (part 1 of 2)

[Borislav Petkov <bp@alien8.de>]

On Wed, Dec 08, 2010 at 03:35:40PM -0800, Luck, Tony wrote:
> > > what happens if it IS_ERR?
> > 
> > Good point - I need to clean up if things fail (and akpm would
> > like to see me re-factor so that there is only one "return"
> > statement for better maintainabiliy).  I'll fix up stuff here.
> 
> Here's what I'll have in the next version. With all the error
> handling the pstore_mkfile() routine was getting a bit big, so
> I split out the writing-to-the-file part into another function.
> The pair of functions now look like this:
> 
> /*
>  * Set up a file structure as if we had opened this file and
>  * write our data to it.
>  */
> static int pstore_writefile(struct inode *inode, struct dentry *dentry,
> 	char *data, size_t size)
> {
> 	struct file f;
> 	ssize_t n;
> 	mm_segment_t old_fs = get_fs();
> 
> 	memset(&f, '0', sizeof f);
> 	f.f_mapping = inode->i_mapping;
> 	f.f_path.dentry = dentry;
> 	f.f_path.mnt = pstore_mnt;
> 	f.f_pos = 0;
> 	f.f_op = inode->i_fop;
> 	set_fs(KERNEL_DS);
> 	n = do_sync_write(&f, data, size, &f.f_pos);
> 	set_fs(old_fs);
> 
> 	return n == size;
> }

Good.

> 
> /*
>  * Make a regular file in the root directory of our file system.
>  * Load it up with "size" bytes of data from "buf".
>  * Set the mtime & ctime to the date that this record was originally stored.
>  */
> int pstore_mkfile(char *name, char *data, size_t size, struct timespec time,
> 		  void *private)
> {
> 	struct dentry	*root = pstore_sb->s_root;
> 	struct dentry	*dentry;
> 	struct inode	*inode;
> 	int		rc = 0;
> 
> 	inode = pstore_get_inode(pstore_sb, root->d_inode, S_IFREG | 0444, 0);
> 	if (!inode) {
> 		rc = -ENOMEM;
> 		goto fail;
> 	}
> 	inode->i_private = private;
> 
> 	mutex_lock(&root->d_inode->i_mutex);
> 
> 	dentry = d_alloc_name(root, name);
> 	if (IS_ERR(dentry)) {
> 		mutex_unlock(&root->d_inode->i_mutex);
> 		rc = -ENOSPC;
> 		goto fail1;
> 	}
> 
> 	d_add(dentry, inode);
> 
> 	mutex_unlock(&root->d_inode->i_mutex);
> 
> 	if (!pstore_writefile(inode, dentry, data, size)) {
> 		inode->i_nlink--;
> 		mutex_lock(&root->d_inode->i_mutex);
> 		d_delete(dentry);
> 		dput(dentry);
> 		mutex_unlock(&root->d_inode->i_mutex);
> 		rc = -ENOSPC;
> 		goto fail;
> 	}

don't we have to iput() the inode here too if pstore_writefile() fails?

> 
> 	if (time.tv_sec)
> 		inode->i_mtime = inode->i_ctime = time;
> 
> 	return 0;
> 
> fail1:
> 	iput(inode);
> fail:
> 	return rc;
> }

So this version is ok. However, I'd go and move the error paths to
labels near the end of the function since this makes the main part of
what it does more readable, IMHO. IOW, something like this (untested, of
course):

int pstore_mkfile(char *name, char *data, size_t size, struct timespec time,
                  void *private)
{
        struct dentry   *root = pstore_sb->s_root;
        struct dentry   *dentry;
        struct inode    *inode;
        int             rc;

	rc = -ENOMEM;
        inode = pstore_get_inode(pstore_sb, root->d_inode, S_IFREG | 0444, 0);
        if (!inode)
                goto fail;

        inode->i_private = private;

        mutex_lock(&root->d_inode->i_mutex);

	rc = -ENOSPC;
        dentry = d_alloc_name(root, name);
        if (IS_ERR(dentry))
                goto fail_alloc;

        d_add(dentry, inode);

        mutex_unlock(&root->d_inode->i_mutex);

        if (!pstore_writefile(inode, dentry, data, size))
		goto fail_write;

        if (time.tv_sec)
                inode->i_mtime = inode->i_ctime = time;

        return 0;

fail_write:
	inode->i_nlink--;
	mutex_lock(&root->d_inode->i_mutex);
	d_delete(dentry);
        dput(dentry);
	/* we fall through to unlock the mutex below */

fail_alloc:
	mutex_unlock(&root->d_inode->i_mutex);
        iput(inode);

fail:
        return rc;
}


Hmm.

-- 
Regards/Gruss,
Boris.