| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 23 Jan 2015 11:29:45 +0800
From: Chao Yu <chao2.yu@...sung.com>
To: 'Jaegeuk Kim' <jaegeuk@...nel.org>
Cc: 'Changman Lee' <cm224.lee@...sung.com>,
linux-f2fs-devel@...ts.sourceforge.net,
linux-kernel@...r.kernel.org
Subject: RE: [f2fs-dev][PATCH 2/2] f2fs: enable recover_xattr_data to avoid cp
when fsync after operating xattr
Hi Jaegeuk,
> -----Original Message-----
> From: Jaegeuk Kim [mailto:jaegeuk@...nel.org]
> Sent: Friday, January 23, 2015 7:13 AM
> To: Chao Yu
> Cc: 'Changman Lee'; linux-f2fs-devel@...ts.sourceforge.net; linux-kernel@...r.kernel.org
> Subject: Re: [f2fs-dev][PATCH 2/2] f2fs: enable recover_xattr_data to avoid cp when fsync after
> operating xattr
>
> Hi Chao,
>
> On Wed, Jan 14, 2015 at 01:01:13PM +0800, Chao Yu wrote:
> > Hi Jaegeuk,
> >
> > > -----Original Message-----
> > > From: Jaegeuk Kim [mailto:jaegeuk@...nel.org]
> > > Sent: Wednesday, January 14, 2015 8:22 AM
> > > To: Chao Yu
> > > Cc: 'Changman Lee'; linux-f2fs-devel@...ts.sourceforge.net; linux-kernel@...r.kernel.org
> > > Subject: Re: [f2fs-dev][PATCH 2/2] f2fs: enable recover_xattr_data to avoid cp when fsync
> after
> > > operating xattr
> > >
> > > On Mon, Jan 12, 2015 at 05:40:28PM +0800, Chao Yu wrote:
> > > > Hi Jaegeuk,
> > > >
> > > > > -----Original Message-----
> > > > > From: Jaegeuk Kim [mailto:jaegeuk@...nel.org]
> > > > > Sent: Sunday, January 11, 2015 1:32 PM
> > > > > To: Chao Yu
> > > > > Cc: 'Changman Lee'; linux-f2fs-devel@...ts.sourceforge.net;
> linux-kernel@...r.kernel.org
> > > > > Subject: Re: [f2fs-dev][PATCH 2/2] f2fs: enable recover_xattr_data to avoid cp when
> fsync
> > > after
> > > > > operating xattr
> > > > >
> > > > > On Sat, Jan 10, 2015 at 08:08:33PM +0800, Chao Yu wrote:
> > > > > > Hi Jaegeuk,
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Jaegeuk Kim [mailto:jaegeuk@...nel.org]
> > > > > > > Sent: Wednesday, January 07, 2015 3:44 AM
> > > > > > > To: Chao Yu
> > > > > > > Cc: Changman Lee; linux-f2fs-devel@...ts.sourceforge.net;
> linux-kernel@...r.kernel.org
> > > > > > > Subject: Re: [f2fs-dev][PATCH 2/2] f2fs: enable recover_xattr_data to avoid cp when
> > > fsync
> > > > > after
> > > > > > > operating xattr
> > > > > > >
> > > > > > > Hi Chao,
> > > > > > >
> > > > > > > On Tue, Jan 06, 2015 at 02:29:40PM +0800, Chao Yu wrote:
> > > > > > > > Now if we call fsync() after we update the xattr date belongs to the file, f2fs
> > > > > > > > will do checkpoint to keep data.
> > > > > > > > This can cause low performance because checkpoint block most operation and write
> > > > > > > > lots of blocks. So we'd better to avoid doing checkpoint by writing modified
> > > > > > > > xattr node page to warm node segment, and then it can be recovered when we mount
> > > > > > > > this device later on.
> > > > > > >
> > > > > > > You're trying to change the writing policy as xattr blocks are written into
> > > > > > > WARM_NODE area instead of COLD_NODE area.
> > > > > > > I don't think xattrs are frequently changed between each fsync calls.
> > > > > > >
> > > > > > > How do you think?
> > > > > >
> > > > > > I'm not sure whether there is a scenario that setxattr and fsync are invoked
> > > > > > alternately, but if there is, our performance will decrease obviously.
> > > > > >
> > > > > > If you don't want to change writing policy, how about writing xattr node with
> > > > > > fsync flag into cold node segment when fsync() is called, then try to recover
> > > > > > it from cold node chain when recovery after abnormally pow-cut, this way can
> > > > > > avoid cp frequently in above scenario.
> > > > >
> > > > > Firt of all, I don't think this scenario is frequent enough that we have to
> > > > > break the exisiting writing and recovery procedures.
> > > > > Moreover, if xattr entries are covered by inline_xattr, it doesn't trigger
> > > > > checkpoint.
> > > >
> > > > Agree, that's a good solution.
> > > >
> > > > >
> > > > > Let me know, if I'm missing something.
> > > > >
> > > > > If you try to change the recovery procedure, it needs to think about the
> > > > > disk full condition. (i.e., space_for_roll_forward())
> > > > > And, I don't want to search cold node chain.
> > > >
> > > > OK, if we keep writing policy and recovery procedure as it is, then, shouldn't our
> > > > recover_xattr_data be dropped because it will be not used from any call path?
> > > > How do you think of below patch?
> > >
> > > Hi Chao,
> > >
> > > Nice catch.
> > > But, IIRC, this code was remained for backward compatibility, since long time
> > > ago, xattr blocks were written into the warm node chain.
> >
> > Ah, I got it, thanks for your explanation! :)
> > How do you think of adding some comments on these codes, because this can help
> > developers understand it well and not to submit the wrong fix patch like me again.
>
> Something like this?
It's good, this is what I wanted.
Thanks for your work! :)
>
> From 6b609421e4f9f52de26554300aae62de33e0703a Mon Sep 17 00:00:00 2001
> From: Jaegeuk Kim <jaegeuk@...nel.org>
> Date: Thu, 22 Jan 2015 14:48:28 -0800
> Subject: [PATCH] f2fs: leave comment for code readability
>
> During the recovery, any xattr blocks should not be found, since they are
> written into cold log, not the warm node chain.
>
> Signed-off-by: Jaegeuk Kim <jaegeuk@...nel.org>
Reviewed-by: Chao Yu <chao2.yu@...sung.com>
Thanks,
Yu
> ---
> fs/f2fs/recovery.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
> index c4211a5..57603a7 100644
> --- a/fs/f2fs/recovery.c
> +++ b/fs/f2fs/recovery.c
> @@ -346,6 +346,10 @@ static int do_recover_data(struct f2fs_sb_info *sbi, struct inode *inode,
> if (IS_INODE(page)) {
> recover_inline_xattr(inode, page);
> } else if (f2fs_has_xattr_block(ofs_of_node(page))) {
> + /*
> + * Deprecated; xattr blocks should be found from cold log.
> + * But, we should remain this for backward compatibility.
> + */
> recover_xattr_data(inode, page, blkaddr);
> goto out;
> }
> --
> 2.1.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists