| Message ID | 20201221195055.35295-3-vgoyal@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfs, overlayfs: Fix syncfs() to return correct errors | expand |
On Mon, Dec 21, 2020 at 02:50:54PM -0500, Vivek Goyal wrote: > - ret2 = errseq_check_and_advance(&sb->s_wb_err, &f.file->f_sb_err); > + if (sb->s_op->errseq_check_advance) > + ret2 = sb->s_op->errseq_check_advance(sb, f.file); What a terrible name for an fs operation. You don't seem to be able to distinguish between semantics and implementation. How about check_error()?
On Tue, Dec 22, 2020 at 04:19:00PM +0000, Matthew Wilcox wrote: > On Mon, Dec 21, 2020 at 02:50:54PM -0500, Vivek Goyal wrote: > > - ret2 = errseq_check_and_advance(&sb->s_wb_err, &f.file->f_sb_err); > > + if (sb->s_op->errseq_check_advance) > > + ret2 = sb->s_op->errseq_check_advance(sb, f.file); > > What a terrible name for an fs operation. You don't seem to be able > to distinguish between semantics and implementation. How about > check_error()? check_error() sounds better. I was not very happy with the name either. Thought of starting with something. Vivek
On Tue, 2020-12-22 at 11:25 -0500, Vivek Goyal wrote: > On Tue, Dec 22, 2020 at 04:19:00PM +0000, Matthew Wilcox wrote: > > On Mon, Dec 21, 2020 at 02:50:54PM -0500, Vivek Goyal wrote: > > > - ret2 = errseq_check_and_advance(&sb->s_wb_err, &f.file->f_sb_err); > > > + if (sb->s_op->errseq_check_advance) > > > + ret2 = sb->s_op->errseq_check_advance(sb, f.file); > > > > What a terrible name for an fs operation. You don't seem to be able > > to distinguish between semantics and implementation. How about > > check_error()? > > check_error() sounds better. I was not very happy with the name either. > Thought of starting with something. > Maybe report_error() ? The same error won't be reported on the next call on the same fd. I think the important point to make here is that this error must be reported to syncfs() or something like it once you call this. (In hindsight, I sort of wish I had done s/serrseq_set/errseq_record/ and s/errseq_check_and_advance/errseq_report/ when I initially did this, if only to make the API a little less dependent on the implementation.)
On Mon, 2020-12-21 at 14:50 -0500, Vivek Goyal wrote: > Right now we check for errors on super block in syncfs(). > > ret2 = errseq_check_and_advance(&sb->s_wb_err, &f.file->f_sb_err); > > overlayfs does not update sb->s_wb_err and it is tracked on upper filesystem. > So provide a superblock operation to check errors so that filesystem > can provide override generic method and provide its own method to > check for writeback errors. > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com> > --- > fs/sync.c | 5 ++++- > include/linux/fs.h | 1 + > 2 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/fs/sync.c b/fs/sync.c > index b5fb83a734cd..57e43a16dfca 100644 > --- a/fs/sync.c > +++ b/fs/sync.c > @@ -176,7 +176,10 @@ SYSCALL_DEFINE1(syncfs, int, fd) > ret = sync_filesystem(sb); > up_read(&sb->s_umount); > > > - ret2 = errseq_check_and_advance(&sb->s_wb_err, &f.file->f_sb_err); > + if (sb->s_op->errseq_check_advance) > + ret2 = sb->s_op->errseq_check_advance(sb, f.file); > + else > + ret2 = errseq_check_and_advance(&sb->s_wb_err, &f.file->f_sb_err); > > > fdput(f); > return ret ? ret : ret2; > diff --git a/include/linux/fs.h b/include/linux/fs.h > index 8667d0cdc71e..4297b6127adf 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -1965,6 +1965,7 @@ struct super_operations { > struct shrink_control *); > long (*free_cached_objects)(struct super_block *, > struct shrink_control *); > + int (*errseq_check_advance)(struct super_block *, struct file *); > }; > > > /* Also, the other super_operations generally don't take a superblock pointer when you pass in a different fs object pointer. This should probably just take a struct file * and then the operation can chase pointers to the superblock from there.
On Wed, Dec 23, 2020 at 07:48:52AM -0500, Jeff Layton wrote: > On Mon, 2020-12-21 at 14:50 -0500, Vivek Goyal wrote: > > Right now we check for errors on super block in syncfs(). > > > > ret2 = errseq_check_and_advance(&sb->s_wb_err, &f.file->f_sb_err); > > > > overlayfs does not update sb->s_wb_err and it is tracked on upper filesystem. > > So provide a superblock operation to check errors so that filesystem > > can provide override generic method and provide its own method to > > check for writeback errors. > > > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com> > > --- > > fs/sync.c | 5 ++++- > > include/linux/fs.h | 1 + > > 2 files changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/fs/sync.c b/fs/sync.c > > index b5fb83a734cd..57e43a16dfca 100644 > > --- a/fs/sync.c > > +++ b/fs/sync.c > > @@ -176,7 +176,10 @@ SYSCALL_DEFINE1(syncfs, int, fd) > > ret = sync_filesystem(sb); > > up_read(&sb->s_umount); > > > > > > - ret2 = errseq_check_and_advance(&sb->s_wb_err, &f.file->f_sb_err); > > + if (sb->s_op->errseq_check_advance) > > + ret2 = sb->s_op->errseq_check_advance(sb, f.file); > > + else > > + ret2 = errseq_check_and_advance(&sb->s_wb_err, &f.file->f_sb_err); > > > > > > fdput(f); > > return ret ? ret : ret2; > > diff --git a/include/linux/fs.h b/include/linux/fs.h > > index 8667d0cdc71e..4297b6127adf 100644 > > --- a/include/linux/fs.h > > +++ b/include/linux/fs.h > > @@ -1965,6 +1965,7 @@ struct super_operations { > > struct shrink_control *); > > long (*free_cached_objects)(struct super_block *, > > struct shrink_control *); > > + int (*errseq_check_advance)(struct super_block *, struct file *); > > }; > > > > > > /* > > Also, the other super_operations generally don't take a superblock > pointer when you pass in a different fs object pointer. This should > probably just take a struct file * and then the operation can chase > pointers to the superblock from there. Ok, I will drop super_block * argument and just pass in "struct file *". Vivek > > -- > Jeff Layton <jlayton@kernel.org> >
diff --git a/fs/sync.c b/fs/sync.c index b5fb83a734cd..57e43a16dfca 100644 --- a/fs/sync.c +++ b/fs/sync.c @@ -176,7 +176,10 @@ SYSCALL_DEFINE1(syncfs, int, fd) ret = sync_filesystem(sb); up_read(&sb->s_umount); - ret2 = errseq_check_and_advance(&sb->s_wb_err, &f.file->f_sb_err); + if (sb->s_op->errseq_check_advance) + ret2 = sb->s_op->errseq_check_advance(sb, f.file); + else + ret2 = errseq_check_and_advance(&sb->s_wb_err, &f.file->f_sb_err); fdput(f); return ret ? ret : ret2; diff --git a/include/linux/fs.h b/include/linux/fs.h index 8667d0cdc71e..4297b6127adf 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -1965,6 +1965,7 @@ struct super_operations { struct shrink_control *); long (*free_cached_objects)(struct super_block *, struct shrink_control *); + int (*errseq_check_advance)(struct super_block *, struct file *); }; /*
Right now we check for errors on super block in syncfs(). ret2 = errseq_check_and_advance(&sb->s_wb_err, &f.file->f_sb_err); overlayfs does not update sb->s_wb_err and it is tracked on upper filesystem. So provide a superblock operation to check errors so that filesystem can provide override generic method and provide its own method to check for writeback errors. Signed-off-by: Vivek Goyal <vgoyal@redhat.com> --- fs/sync.c | 5 ++++- include/linux/fs.h | 1 + 2 files changed, 5 insertions(+), 1 deletion(-)