Replace file_start_write with file_start_write_killable where
possible.
Signed-off-by: Marco Stornelli <[email protected]>
Reviewed-by: Jan Kara <[email protected]>
---
drivers/block/loop.c | 4 +++-
fs/aio.c | 7 +++++--
fs/coda/file.c | 4 +++-
fs/read_write.c | 28 +++++++++++++++++-----------
fs/splice.c | 4 +++-
include/linux/fs.h | 17 +++++++++++++++++
6 files changed, 48 insertions(+), 16 deletions(-)
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index be9a101..2c0d0a3 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -230,7 +230,9 @@ static int __do_lo_send_write(struct file *file,
ssize_t bw;
mm_segment_t old_fs = get_fs();
- file_start_write(file);
+ bw = file_start_write_killable(file);
+ if (bw < 0)
+ return bw;
set_fs(get_ds());
bw = file->f_op->write(file, buf, len, &pos);
set_fs(old_fs);
diff --git a/fs/aio.c b/fs/aio.c
index 5b7ed78..5deddf5 100644
--- a/fs/aio.c
+++ 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 {
ret = rw_op(iocb, &iocb->ki_iovec[iocb->ki_cur_seg],
iocb->ki_nr_segs - iocb->ki_cur_seg,
diff --git a/fs/coda/file.c b/fs/coda/file.c
index 380b798..c5708d0 100644
--- a/fs/coda/file.c
+++ b/fs/coda/file.c
@@ -79,7 +79,9 @@ coda_file_write(struct file *coda_file, const char __user *buf, size_t count, lo
return -EINVAL;
host_inode = file_inode(host_file);
- file_start_write(host_file);
+ ret = file_start_write_killable(host_file);
+ if (ret < 0)
+ return ret;
mutex_lock(&coda_inode->i_mutex);
ret = host_file->f_op->write(host_file, buf, count, ppos);
diff --git a/fs/read_write.c b/fs/read_write.c
index 7eb7ef3..ed9006f 100644
--- a/fs/read_write.c
+++ 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;
@@ -718,7 +720,9 @@ static ssize_t do_readv_writev(int type, struct file *file,
} else {
fn = (io_fn_t)file->f_op->write;
fnv = file->f_op->aio_write;
- file_start_write(file);
+ ret = file_start_write_killable(file);
+ if (ret < 0)
+ goto out;
}
if (fnv)
@@ -898,7 +902,9 @@ static ssize_t compat_do_readv_writev(int type, struct file *file,
} else {
fn = (io_fn_t)file->f_op->write;
fnv = file->f_op->aio_write;
- file_start_write(file);
+ ret = file_start_write_killable(file);
+ if (ret < 0)
+ goto out;
}
if (fnv)
diff --git a/fs/splice.c b/fs/splice.c
index e6b2559..b37c30e 100644
--- a/fs/splice.c
+++ b/fs/splice.c
@@ -1115,7 +1115,9 @@ static long do_splice_from(struct pipe_inode_info *pipe, struct file *out,
else
splice_write = default_file_splice_write;
- file_start_write(out);
+ ret = file_start_write_killable(out);
+ if (ret < 0)
+ return ret;
ret = splice_write(pipe, out, ppos, len, flags);
file_end_write(out);
return ret;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index c8b7325..998ec2a 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1404,6 +1404,16 @@ static inline void sb_start_write(struct super_block *sb)
__sb_start_write(sb, SB_FREEZE_WRITE, FREEZE_WAIT);
}
+/**
+ * sb_start_write_killable - get write access to a superblock
+ * @sb: the super we write to
+ *
+ */
+static inline int sb_start_write_killable(struct super_block *sb)
+{
+ return __sb_start_write(sb, SB_FREEZE_WRITE, FREEZE_WAIT_KILLABLE);
+}
+
static inline int sb_start_write_trylock(struct super_block *sb)
{
return __sb_start_write(sb, SB_FREEZE_WRITE, FREEZE_NOWAIT);
@@ -2227,6 +2237,13 @@ static inline struct inode *file_inode(struct file *f)
return f->f_inode;
}
+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);
+}
+
static inline void file_start_write(struct file *file)
{
if (!S_ISREG(file_inode(file)->i_mode))
--
1.7.3.4
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."
Hi,
Il 26/04/2013 14:06, Matthew Wilcox ha scritto:
> 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:
The patch series is based on -next due to several changes done by Al
about fsfreeze. file_start_write_killable returns 1 because it's mainly
a wrapper of __st_start_write. __sb_start_write returns 1 when
everything is ok, 0 when the lock can't be gotten (we are using the
trylock version) and _now_ a value < 0 when something happens (i.e. -EINTR).
>
>> +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.
>
See above.
>> +++ 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:
>
Ok, I can change the style here, no problem.
Marco