[PATCH 3/5] f2fs: report readonly status in ->fsync

[PATCH 3/5] f2fs: report readonly status in ->fsync

[Chao Yu]

Report readonly status of filesystem during fsync.

Signed-off-by: Chao Yu <chao2.yu@samsung.com>
---
 fs/f2fs/file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 2d87a3c..a60d088 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -197,7 +197,7 @@ int f2fs_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
 	};
 
 	if (unlikely(f2fs_readonly(inode->i_sb)))
-		return 0;
+		return -EROFS;
 
 	trace_f2fs_sync_file_enter(inode);
 
-- 
2.6.3

Re: [PATCH 3/5] f2fs: report readonly status in ->fsync

[Marc Lehmann]

On Thu, Dec 24, 2015 at 06:07:25PM +0800, Chao Yu <chao2.yu@samsung.com> wrote:
> Report readonly status of filesystem during fsync.

If this means that fsync returns EROFS when used on a file on a read-only
filesystem., then this looks buggy - EROFS is not documented for
fsync (only as "fd is bound to a special file which does not support
synchronization") for this condition, and I don't think other filesystems
do that (just tried xfs).

It also doesn't quite make sense - fsync should only fail when the file
couldn't be synced, but in most (probably all) cases, the file is already
synchronised, otherwise the filesystem shouldn't be RO.

I only looked at the diff, so if I missed the context and my reasoning
doesn't apply, forgive and ignore me :)

-- 
                The choice of a       Deliantra, the free code+content MORPG
      -----==-     _GNU_              http://www.deliantra.net
      ----==-- _       generation
      ---==---(_)__  __ ____  __      Marc Lehmann
      --==---/ / _ \/ // /\ \/ /      schmorp@schmorp.de
      -=====/_/_//_/\_,_/ /_/\_\

------------------------------------------------------------------------------

Re: [PATCH 3/5] f2fs: report readonly status in ->fsync

[Chao Yu]

> -----Original Message-----
> From: Marc Lehmann [mailto:schmorp@schmorp.de]
> Sent: Saturday, December 26, 2015 2:33 PM
> To: Chao Yu
> Cc: linux-f2fs-devel@lists.sourceforge.net
> Subject: Re: [f2fs-dev] [PATCH 3/5] f2fs: report readonly status in ->fsync
> 
> On Thu, Dec 24, 2015 at 06:07:25PM +0800, Chao Yu <chao2.yu@samsung.com> wrote:
> > Report readonly status of filesystem during fsync.
> 
> If this means that fsync returns EROFS when used on a file on a read-only
> filesystem., then this looks buggy - EROFS is not documented for
> fsync (only as "fd is bound to a special file which does not support
> synchronization") for this condition, and I don't think other filesystems
> do that (just tried xfs).

Yes.

> 
> It also doesn't quite make sense - fsync should only fail when the file
> couldn't be synced, but in most (probably all) cases, the file is already
> synchronised, otherwise the filesystem shouldn't be RO.

Agreed,

One situation here is if '.' or '..' in dentry page of directory inode was
missing (corrupted directory), once fsck found this kind of inode exist in
the image, it will mark inode with F2FS_INLINE_DOTS flag, after that f2fs
will try to recover it to normal one if user touch such inode in ->lookup,
so the dirty data was generated even in a readonly fs.

Actually even for above case, it would be better to avoid generating dirty
data rather than stop user fsync in a readonly fs.

Hi Jaegeuk, could you help to drop this patch?

And one more thing is how do you think of moving inline dot recovery into
fsck?

Thanks,

> 
> I only looked at the diff, so if I missed the context and my reasoning
> doesn't apply, forgive and ignore me :)
> 
> --
>                 The choice of a       Deliantra, the free code+content MORPG
>       -----==-     _GNU_              http://www.deliantra.net
>       ----==-- _       generation
>       ---==---(_)__  __ ____  __      Marc Lehmann
>       --==---/ / _ \/ // /\ \/ /      schmorp@schmorp.de
>       -=====/_/_//_/\_,_/ /_/\_\


------------------------------------------------------------------------------

Re: [PATCH 3/5] f2fs: report readonly status in ->fsync

[Jaegeuk Kim]

Hi Chao,

Thanks Marc for your report.

On Mon, Dec 28, 2015 at 05:59:19PM +0800, Chao Yu wrote:
> > -----Original Message-----
> > From: Marc Lehmann [mailto:schmorp@schmorp.de]
> > Sent: Saturday, December 26, 2015 2:33 PM
> > To: Chao Yu
> > Cc: linux-f2fs-devel@lists.sourceforge.net
> > Subject: Re: [f2fs-dev] [PATCH 3/5] f2fs: report readonly status in ->fsync
> > 
> > On Thu, Dec 24, 2015 at 06:07:25PM +0800, Chao Yu <chao2.yu@samsung.com> wrote:
> > > Report readonly status of filesystem during fsync.
> > 
> > If this means that fsync returns EROFS when used on a file on a read-only
> > filesystem., then this looks buggy - EROFS is not documented for
> > fsync (only as "fd is bound to a special file which does not support
> > synchronization") for this condition, and I don't think other filesystems
> > do that (just tried xfs).
> 
> Yes.
> 
> > 
> > It also doesn't quite make sense - fsync should only fail when the file
> > couldn't be synced, but in most (probably all) cases, the file is already
> > synchronised, otherwise the filesystem shouldn't be RO.
> 
> Agreed,
> 
> One situation here is if '.' or '..' in dentry page of directory inode was
> missing (corrupted directory), once fsck found this kind of inode exist in
> the image, it will mark inode with F2FS_INLINE_DOTS flag, after that f2fs
> will try to recover it to normal one if user touch such inode in ->lookup,
> so the dirty data was generated even in a readonly fs.
> 
> Actually even for above case, it would be better to avoid generating dirty
> data rather than stop user fsync in a readonly fs.
> 
> Hi Jaegeuk, could you help to drop this patch?

Okay.

> 
> And one more thing is how do you think of moving inline dot recovery into
> fsck?

Hmm, we can't move that part entirely into fsck regarding to backward
compatibility. We can add that in fsck, but it needs another block
allocation part which we need to handle -ENOSPC in fsck as well.

Thanks,

> 
> Thanks,
> 
> > 
> > I only looked at the diff, so if I missed the context and my reasoning
> > doesn't apply, forgive and ignore me :)
> > 
> > --
> >                 The choice of a       Deliantra, the free code+content MORPG
> >       -----==-     _GNU_              http://www.deliantra.net
> >       ----==-- _       generation
> >       ---==---(_)__  __ ____  __      Marc Lehmann
> >       --==---/ / _ \/ // /\ \/ /      schmorp@schmorp.de
> >       -=====/_/_//_/\_,_/ /_/\_\

------------------------------------------------------------------------------

Re: [PATCH 3/5] f2fs: report readonly status in ->fsync

[Chao Yu]

Hi Jaegeuk,

> -----Original Message-----
> From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> Sent: Tuesday, December 29, 2015 6:44 AM
> To: Chao Yu
> Cc: 'Marc Lehmann'; linux-f2fs-devel@lists.sourceforge.net
> Subject: Re: [f2fs-dev] [PATCH 3/5] f2fs: report readonly status in ->fsync
> 
> Hi Chao,
> 
> Thanks Marc for your report.
> 
> On Mon, Dec 28, 2015 at 05:59:19PM +0800, Chao Yu wrote:
> > > -----Original Message-----
> > > From: Marc Lehmann [mailto:schmorp@schmorp.de]
> > > Sent: Saturday, December 26, 2015 2:33 PM
> > > To: Chao Yu
> > > Cc: linux-f2fs-devel@lists.sourceforge.net
> > > Subject: Re: [f2fs-dev] [PATCH 3/5] f2fs: report readonly status in ->fsync
> > >
> > > On Thu, Dec 24, 2015 at 06:07:25PM +0800, Chao Yu <chao2.yu@samsung.com> wrote:
> > > > Report readonly status of filesystem during fsync.
> > >
> > > If this means that fsync returns EROFS when used on a file on a read-only
> > > filesystem., then this looks buggy - EROFS is not documented for
> > > fsync (only as "fd is bound to a special file which does not support
> > > synchronization") for this condition, and I don't think other filesystems
> > > do that (just tried xfs).
> >
> > Yes.
> >
> > >
> > > It also doesn't quite make sense - fsync should only fail when the file
> > > couldn't be synced, but in most (probably all) cases, the file is already
> > > synchronised, otherwise the filesystem shouldn't be RO.
> >
> > Agreed,
> >
> > One situation here is if '.' or '..' in dentry page of directory inode was
> > missing (corrupted directory), once fsck found this kind of inode exist in
> > the image, it will mark inode with F2FS_INLINE_DOTS flag, after that f2fs
> > will try to recover it to normal one if user touch such inode in ->lookup,
> > so the dirty data was generated even in a readonly fs.
> >
> > Actually even for above case, it would be better to avoid generating dirty
> > data rather than stop user fsync in a readonly fs.
> >
> > Hi Jaegeuk, could you help to drop this patch?
> 
> Okay.
> 
> >
> > And one more thing is how do you think of moving inline dot recovery into
> > fsck?
> 
> Hmm, we can't move that part entirely into fsck regarding to backward
> compatibility.

IMO, dynamical repair could be optional, not a must. Because when touching
such incomplete directory in ->lookup, we could leave some dmesg to remind
user to repair. Or if user found this issue by himself, also he could
repair it by using fsck. So this shouldn't case a backward compatibility
issue, or am I missing something?

> We can add that in fsck, but it needs another block
> allocation part which we need to handle -ENOSPC in fsck as well.

That's right, is it possible to do defragment (gc) if there is no
more space when allocating block?

Thanks,

> 
> Thanks,
> 
> >
> > Thanks,
> >
> > >
> > > I only looked at the diff, so if I missed the context and my reasoning
> > > doesn't apply, forgive and ignore me :)
> > >
> > > --
> > >                 The choice of a       Deliantra, the free code+content MORPG
> > >       -----==-     _GNU_              http://www.deliantra.net
> > >       ----==-- _       generation
> > >       ---==---(_)__  __ ____  __      Marc Lehmann
> > >       --==---/ / _ \/ // /\ \/ /      schmorp@schmorp.de
> > >       -=====/_/_//_/\_,_/ /_/\_\


------------------------------------------------------------------------------

Re: [PATCH 3/5] f2fs: report readonly status in ->fsync

[Jaegeuk Kim]

Hi Chao,

...

> > > And one more thing is how do you think of moving inline dot recovery into
> > > fsck?
> > 
> > Hmm, we can't move that part entirely into fsck regarding to backward
> > compatibility.
> 
> IMO, dynamical repair could be optional, not a must. Because when touching
> such incomplete directory in ->lookup, we could leave some dmesg to remind
> user to repair. Or if user found this issue by himself, also he could
> repair it by using fsck. So this shouldn't case a backward compatibility
> issue, or am I missing something?

Hmm, it seems that maybe I misunderstood your *moving it into fsck*.
Anyway, I'm fine to add kernel messages as well.
And, in any case, the only way to fix this case is to run fsck at first.
One approach, as I implemented, is to set a flag and then repair it at runtime.

The other one is to repair by fsck, as you suggested.
In this case, my concern is that current fsck is working without allocating
blocks, since it is a little bit risky right now.
For example, if there is no valid user block, not free space, fsck should handle
ENOSPC while changing the existing disk, which has not been fully validated so
far.

Ah, otherwise, we can activate inline_dentry by fsck for this special case, once
inline_dentry in f2fs becomes pretty much stable later. :)

Thanks,

> 
> > We can add that in fsck, but it needs another block
> > allocation part which we need to handle -ENOSPC in fsck as well.
> 
> That's right, is it possible to do defragment (gc) if there is no
> more space when allocating block?
> 
> Thanks,
> 
> > 
> > Thanks,
> > 
> > >
> > > Thanks,
> > >
> > > >
> > > > I only looked at the diff, so if I missed the context and my reasoning
> > > > doesn't apply, forgive and ignore me :)
> > > >
> > > > --
> > > >                 The choice of a       Deliantra, the free code+content MORPG
> > > >       -----==-     _GNU_              http://www.deliantra.net
> > > >       ----==-- _       generation
> > > >       ---==---(_)__  __ ____  __      Marc Lehmann
> > > >       --==---/ / _ \/ // /\ \/ /      schmorp@schmorp.de
> > > >       -=====/_/_//_/\_,_/ /_/\_\

------------------------------------------------------------------------------

Re: [PATCH 3/5] f2fs: report readonly status in ->fsync

[Chao Yu]

Hi Jaegeuk,

> -----Original Message-----
> From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> Sent: Wednesday, December 30, 2015 8:33 AM
> To: Chao Yu
> Cc: 'Marc Lehmann'; linux-f2fs-devel@lists.sourceforge.net
> Subject: Re: [f2fs-dev] [PATCH 3/5] f2fs: report readonly status in ->fsync
> 
> Hi Chao,
> 
> ...
> 
> > > > And one more thing is how do you think of moving inline dot recovery into
> > > > fsck?
> > >
> > > Hmm, we can't move that part entirely into fsck regarding to backward
> > > compatibility.
> >
> > IMO, dynamical repair could be optional, not a must. Because when touching
> > such incomplete directory in ->lookup, we could leave some dmesg to remind
> > user to repair. Or if user found this issue by himself, also he could
> > repair it by using fsck. So this shouldn't case a backward compatibility
> > issue, or am I missing something?
> 
> Hmm, it seems that maybe I misunderstood your *moving it into fsck*.

What I mean is 1) enable repair ability in fsck, 2) keep repair in kernel
side optionally. For compatibility, we leave dmesg for user when touch this
kind of dir if we choose to disable repair in kernel.

> Anyway, I'm fine to add kernel messages as well.

OK, will do it in ("f2fs: fix to stop recovering dot dentries in a readonly fs")

> And, in any case, the only way to fix this case is to run fsck at first.
> One approach, as I implemented, is to set a flag and then repair it at runtime.
> 
> The other one is to repair by fsck, as you suggested.
> In this case, my concern is that current fsck is working without allocating
> blocks, since it is a little bit risky right now.
> For example, if there is no valid user block, not free space, fsck should handle
> ENOSPC while changing the existing disk, which has not been fully validated so
> far.

That' right.

> 
> Ah, otherwise, we can activate inline_dentry by fsck for this special case, once
> inline_dentry in f2fs becomes pretty much stable later. :)

Or we can active inline_data for file which has i_size less than MAX_INLINE_SIZE.

That will be cool to use inline_dentry by default! :)

Thanks,

> 
> Thanks,
> 
> >
> > > We can add that in fsck, but it needs another block
> > > allocation part which we need to handle -ENOSPC in fsck as well.
> >
> > That's right, is it possible to do defragment (gc) if there is no
> > more space when allocating block?
> >
> > Thanks,
> >
> > >
> > > Thanks,
> > >
> > > >
> > > > Thanks,
> > > >
> > > > >
> > > > > I only looked at the diff, so if I missed the context and my reasoning
> > > > > doesn't apply, forgive and ignore me :)
> > > > >
> > > > > --
> > > > >                 The choice of a       Deliantra, the free code+content MORPG
> > > > >       -----==-     _GNU_              http://www.deliantra.net
> > > > >       ----==-- _       generation
> > > > >       ---==---(_)__  __ ____  __      Marc Lehmann
> > > > >       --==---/ / _ \/ // /\ \/ /      schmorp@schmorp.de
> > > > >       -=====/_/_//_/\_,_/ /_/\_\


------------------------------------------------------------------------------