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

[Chao Yu <chao2.yu@samsung.com>]

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
> > >       -=====/_/_//_/\_,_/ /_/\_\


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