On Fri, 24 Jan 2014 14:44:38 +0100, Andreas Rohner wrote:
> On 2014-01-24 14:20, Ryusuke Konishi wrote:
>> On Tue, 21 Jan 2014 15:00:30 +0100, Andreas Rohner wrote:
>>> With this ioctl the segment usage information in the SUFILE can be
>>> updated from userspace.
>>>
>>> This is useful, because it allows the userspace GC to modify and update
>>> segment usage entries for specific segments, which enables it to avoid
>>> unnecessary write operations.
>>>
>>> If a segment needs to be cleaned, but there is no or very little free
>>> space to be gained, the cleaning operation basically degrades to
>>> needless expensive copying of data. In the end the only thing that
>>> changes is the location of the data and a timestamp in the segment usage
>>> info. With this ioctl the GC can skip the copying and update the segment
>>> usage entries directly instead.
>>>
>>> Signed-off-by: Andreas Rohner <andreas.rohner@xxxxxxx>
>>> ---
>>> fs/nilfs2/ioctl.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++
>>> include/linux/nilfs2_fs.h | 2 ++
>>> 2 files changed, 52 insertions(+)
>>>
>>> diff --git a/fs/nilfs2/ioctl.c b/fs/nilfs2/ioctl.c
>>> index b44bdb2..c6a3faf 100644
>>> --- a/fs/nilfs2/ioctl.c
>>> +++ b/fs/nilfs2/ioctl.c
>>> @@ -273,6 +273,13 @@ nilfs_ioctl_do_get_suinfo(struct the_nilfs *nilfs, __u64 *posp, int flags,
>>> return ret;
>>> }
>>>
>>> +static ssize_t
>>> +nilfs_ioctl_do_set_suinfo(struct the_nilfs *nilfs, __u64 *posp, int flags,
>>> + void *buf, size_t size, size_t nmembs)
>>> +{
>>> + return nilfs_sufile_set_suinfo(nilfs->ns_sufile, buf, size, nmembs);
>>> +}
>>> +
>>> static int nilfs_ioctl_get_sustat(struct inode *inode, struct file *filp,
>>> unsigned int cmd, void __user *argp)
>>> {
>>> @@ -767,6 +774,44 @@ out:
>>> return ret;
>>> }
>>>
>>> +static int nilfs_ioctl_set_info(struct inode *inode, struct file *filp,
>>> + unsigned int cmd, void __user *argp,
>>> + size_t membsz,
>>> + ssize_t (*dofunc)(struct the_nilfs *,
>>> + __u64 *, int,
>>> + void *, size_t, size_t))
>>> +{
>>> + struct the_nilfs *nilfs = inode->i_sb->s_fs_info;
>>> + struct nilfs_transaction_info ti;
>>> + struct nilfs_argv argv;
>>> + int ret;
>>> +
>>> + if (!capable(CAP_SYS_ADMIN))
>>> + return -EPERM;
>>> +
>>> + ret = mnt_want_write_file(filp);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + ret = -EFAULT;
>>> + if (copy_from_user(&argv, argp, sizeof(argv)))
>>> + goto out;
>>> +
>>> + if (argv.v_size < membsz)
>>> + return -EINVAL;
>>> +
>>> + nilfs_transaction_begin(inode->i_sb, &ti, 0);
>>> + ret = nilfs_ioctl_wrap_copy(nilfs, &argv, _IOC_DIR(cmd), dofunc);
>>
>> Hmm, we have a problem here.
>>
>> nilfs_ioctl_wrap_copy() cannot be used between
>> nilfs_transcation_begin() and nilfs_transaction_end() since
>> nilfs_ioctl_wrap_copy() calls copy_from_user() and/or copy_to_user().
>> Unfortunately there is a lock order restriction for these functions.
>>
>> Let's use vmalloc() like nilfs_ioctl_clean_segments(). The following
>> is a sample code for this:
>>
>> size_t len;
>> void __user *base;
>> void *kbuf;
>> int ret;
>>
>> ...
>> ret = -EINVAL;
>> if (argv.v_size < sizeof(struct nilfs_suinfo_update))
>> goto out;
>>
>> < range check of argv.v_nmembs >
>>
>> len = argv.v_size * argv.v_nmembs;
>> base = (void __user *)(unsigned long)argv.v_base;
>> kbuf = vmalloc(len);
>> if (!kbuf) {
>> ret = -ENOMEM;
>> goto out;
>> }
>>
>> if (copy_from_user(kbuf, base, len)) {
>> ret = -EFAULT;
>> goto out_free;
>> }
>>
>> nilfs_transaction_begin(inode->i_sb, &ti, 0);
>> ret = < call nilfs_sufile_set_suinfo > ;
>> if (unlikely(ret < 0))
>> nilfs_transcation_abort(inode->i_sb);
>> else
>> nilfs_transcation_commit(inode->i_sb);
>>
>> out_free:
>> vfree(kbuf);
>> out:
>> mnt_drop_write_file(filp);
>> ...
>>
>
> Then we don't need the ugly nilfs_suinfo_update structure. I can just
> send an array of struct nilfs_argv[2]. First element has the segment
> numbers and second element has the nilfs_suinfo structures and flags are
> in v_flags. Would that be acceptable?
Personally, I don't like nilfs_argv[n]. Unfortunately, we couldn't
find a good way to pass several array arguments together for GC.
For NILFS_IOCTL_SET_SUINFO, using nilfs_suinfo_update struct looks
better to me from the viewpoint of memory access locality and
readability (and clarity) of source code.
Regards,
Ryusuke Konishi
> br,
> Andreas Rohner
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
