On Fri, Apr 26, 2013 at 10:50:52AM +0200, Marco Stornelli wrote:
> Replace file_start_write with file_start_write_killable where
> possible.
I feel like I'm missing context here. Possibly because you only cc'd me
on patch 2/4. In particular, file_start_write doesn't exist upstream,
so I'm not sure what it's for. But returning 1 for non-regular files
looks dodgy:
> +static inline int file_start_write_killable(struct file *file)
> +{
> + if (!S_ISREG(file_inode(file)->i_mode))
> + return 1;
> + return sb_start_write_killable(file_inode(file)->i_sb);
> +}
> +++ b/fs/aio.c
> @@ -1103,8 +1103,11 @@ static ssize_t aio_rw_vect_retry(struct kiocb *iocb,
> int rw, aio_rw_op *rw_op)
> if (iocb->ki_pos < 0)
> return -EINVAL;
>
> - if (rw == WRITE)
> - file_start_write(file);
> + if (rw == WRITE) {
> + ret = file_start_write_killable(file);
> + if (ret < 0)
> + return ret;
> + }
> do {
So ... it's OK to do this write to pipes/directories/devices/... ? Or is
that check always taken care of elsewhere? If so, why do we need this
check? I'm confused. None of the callers check for the 'ret == 1' case,
so I'm sure there's something wrong here, I just can't tell what it is.
> +++ b/fs/read_write.c
> @@ -438,17 +438,19 @@ ssize_t vfs_write(struct file *file, const char __user
> *buf, size_t count, loff_
> ret = rw_verify_area(WRITE, file, pos, count);
> if (ret >= 0) {
> count = ret;
> - file_start_write(file);
> - if (file->f_op->write)
> - ret = file->f_op->write(file, buf, count, pos);
> - else
> - ret = do_sync_write(file, buf, count, pos);
> + ret = file_start_write_killable(file);
> if (ret > 0) {
> - fsnotify_modify(file);
> - add_wchar(current, ret);
> + if (file->f_op->write)
> + ret = file->f_op->write(file, buf, count, pos);
> + else
> + ret = do_sync_write(file, buf, count, pos);
> + if (ret > 0) {
> + fsnotify_modify(file);
> + add_wchar(current, ret);
> + }
> + inc_syscw(current);
> + file_end_write(file);
> }
> - inc_syscw(current);
> - file_end_write(file);
> }
>
> return ret;
I don't like it that you've increased the indentation here. Better to do
a preliminary patch which just converts to our normal style with gotos. ie:
- if (ret >= 0) {
- count = ret;
- file_start_write(file);
- if (file->f_op->write)
- ret = file->f_op->write(file, buf, count, pos);
- else
- ret = do_sync_write(file, buf, count, pos);
- if (ret > 0) {
- fsnotify_modify(file);
- add_wchar(current, ret);
- }
- inc_syscw(current);
- file_end_write(file);
+ if (ret < 0)
+ goto out;
+ count = ret;
+ file_start_write(file);
+ if (file->f_op->write)
+ ret = file->f_op->write(file, buf, count, pos);
+ else
+ ret = do_sync_write(file, buf, count, pos);
+ if (ret > 0) {
+ fsnotify_modify(file);
+ add_wchar(current, ret);
}
+ inc_syscw(current);
+ file_end_write(file);
+ out:
return ret;
and then this patch would just add another if ... goto out case.
--
Matthew Wilcox Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
