Hi,
On Mon, Dec 14, 2015 at 05:56:49PM +0800, He YunLei wrote:
> On 2015/12/9 2:19, Jaegeuk Kim wrote:
> >Hi Yunlei,
> >
> >On Tue, Dec 08, 2015 at 09:17:13PM +0800, Yunlei He wrote:
> >>write_checkpoint() tries to get cp_blkaddr from superblock buffer,
> >>if the buffer happen to be destroied by something else, it may
> >>bring in unpredictable effect on f2fs.
> >>
> >>this patch fix it by back-up a raw_super copy.
> >>
> >>Signed-off-by: Yunlei He <heyun...@huawei.com>
> >>---
> >> fs/f2fs/checkpoint.c | 3 ++-
> >> fs/f2fs/f2fs.h | 10 ++++++++++
> >> fs/f2fs/node.c | 2 +-
> >> fs/f2fs/super.c | 12 ++++++++----
> >> 4 files changed, 21 insertions(+), 6 deletions(-)
> >>
> >>diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> >>index f661d80..1e342fd 100644
> >>--- a/fs/f2fs/checkpoint.c
> >>+++ b/fs/f2fs/checkpoint.c
> >>@@ -276,13 +276,14 @@ long sync_meta_pages(struct f2fs_sb_info *sbi, enum
> >>page_type type,
> >> long nr_to_write)
> >> {
> >> struct address_space *mapping = META_MAPPING(sbi);
> >>- pgoff_t index = 0, end = LONG_MAX, prev = LONG_MAX;
> >>+ pgoff_t index, end = LONG_MAX, prev = LONG_MAX;
> >> struct pagevec pvec;
> >> long nwritten = 0;
> >> struct writeback_control wbc = {
> >> .for_reclaim = 0,
> >> };
> >>
> >>+ index = __start_cp_addr(sbi);
> >
> >No need to do this.
> >
> >> pagevec_init(&pvec, 0);
> >>
> >> while (index <= end) {
> >>diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> >>index 0052ae8..6afb56c 100644
> >>--- a/fs/f2fs/f2fs.h
> >>+++ b/fs/f2fs/f2fs.h
> >>@@ -1167,6 +1167,16 @@ static inline block_t __start_sum_addr(struct
> >>f2fs_sb_info *sbi)
> >> return le32_to_cpu(F2FS_CKPT(sbi)->cp_pack_start_sum);
> >> }
> >>
> >>+static inline block_t __start_main_addr(struct f2fs_sb_info *sbi)
> >>+{
> >>+ return le32_to_cpu(F2FS_RAW_SUPER(sbi)->main_blkaddr);
> >>+}
> >>+
> >>+static inline block_t __start_sum_addr(struct f2fs_sb_info *sbi)
> >>+{
> >>+ return le32_to_cpu(F2FS_CKPT(sbi)->cp_pack_start_sum);
> >>+}
> >>+
> >
> >Ditto.
> >
> >> static inline bool inc_valid_node_count(struct f2fs_sb_info *sbi,
> >> struct inode *inode)
> >> {
> >>diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> >>index 7bcbc6e..7d2b0bb 100644
> >>--- a/fs/f2fs/node.c
> >>+++ b/fs/f2fs/node.c
> >>@@ -1161,7 +1161,7 @@ int sync_node_pages(struct f2fs_sb_info *sbi, nid_t
> >>ino,
> >> pagevec_init(&pvec, 0);
> >>
> >> next_step:
> >>- index = 0;
> >>+ index = __start_main_addr(sbi);
> >
> >No, its index is for nid.
> >
> >> end = LONG_MAX;
> >>
> >> while (index <= end) {
> >>diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> >>index bd7e9c6..d394711 100644
> >>--- a/fs/f2fs/super.c
> >>+++ b/fs/f2fs/super.c
> >>@@ -1040,6 +1040,9 @@ static int read_raw_super_block(struct super_block
> >>*sb,
> >> struct f2fs_super_block *super;
> >> int err = 0;
> >>
> >>+ super = kzalloc(sizeof(struct f2fs_super_block), GFP_KERNEL);
> >>+ if (!super)
> >>+ return -ENOMEM;
> >> retry:
> >> buffer = sb_bread(sb, block);
> >> if (!buffer) {
> >>@@ -1055,9 +1058,8 @@ retry:
> >> }
> >> }
> >>
> >>- super = (struct f2fs_super_block *)
> >>- ((char *)(buffer)->b_data + F2FS_SUPER_OFFSET);
> >>-
> >>+ memcpy(super, buffer->b_data + F2FS_SUPER_OFFSET,
> >>+ sizeof(struct f2fs_super_block));
> >> /* sanity checking of raw super */
> >> if (sanity_check_raw_super(sb, super)) {
> >> brelse(buffer);
> >>@@ -1090,8 +1092,10 @@ retry:
> >>
> >> out:
> >> /* No valid superblock */
> >>- if (!*raw_super)
> >>+ if (!*raw_super) {
> >>+ kfree(super);
> >> return err;
> >>+ }
> >
> >Need to consider f2fs_commit_super and kfree() in put_super.
>
> We also need to consider f2fs_fill_super error processing procedure:
>
> @@ -1388,6 +1388,7 @@ free_options:
> kfree(options);
> free_sb_buf:
> brelse(raw_super_buf);
> + kfree(raw_super);
Agreed.
>
> >
> >Could you check out this patch?
> >
> >---
> > fs/f2fs/super.c | 18 ++++++++++++++----
> > 1 file changed, 14 insertions(+), 4 deletions(-)
> >
> >diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> >index dbf16ad..8541c93 100644
> >--- a/fs/f2fs/super.c
> >+++ b/fs/f2fs/super.c
> >@@ -567,6 +567,7 @@ static void f2fs_put_super(struct super_block *sb)
> >
> > sb->s_fs_info = NULL;
> > brelse(sbi->raw_super_buf);
> >+ kfree(sbi->raw_super);
> > kfree(sbi);
> > }
> >
> >@@ -1040,6 +1041,9 @@ static int read_raw_super_block(struct super_block *sb,
> > struct f2fs_super_block *super;
> > int err = 0;
> >
> >+ super = kzalloc(sizeof(struct f2fs_super_block), GFP_KERNEL);
> >+ if (!super)
> >+ return -ENOMEM;
> > retry:
> > buffer = sb_bread(sb, block);
> > if (!buffer) {
> >@@ -1055,8 +1059,7 @@ retry:
> > }
> > }
> >
> >- super = (struct f2fs_super_block *)
> >- ((char *)(buffer)->b_data + F2FS_SUPER_OFFSET);
> >+ memcpy(super, buffer->b_data + F2FS_SUPER_OFFSET, sizeof(*super));
> >
> > /* sanity checking of raw super */
> > if (sanity_check_raw_super(sb, super)) {
> >@@ -1090,14 +1093,17 @@ retry:
> >
> > out:
> > /* No valid superblock */
> >- if (!*raw_super)
> >+ if (!*raw_super) {
> >+ kfree(super);
> > return err;
> >+ }
> >
> > return 0;
> > }
> >
> > int f2fs_commit_super(struct f2fs_sb_info *sbi, bool recover)
> > {
> >+ struct f2fs_super_block *super = F2FS_RAW_SUPER(sbi);
> > struct buffer_head *sbh = sbi->raw_super_buf;
> > struct buffer_head *bh;
> > int err;
> >@@ -1108,7 +1114,7 @@ int f2fs_commit_super(struct f2fs_sb_info *sbi, bool
> >recover)
> > return -EIO;
> >
> > lock_buffer(bh);
> >- memcpy(bh->b_data, sbh->b_data, sbh->b_size);
> >+ memcpy(bh->b_data + F2FS_SUPER_OFFSET, super, sizeof(*super));
> > WARN_ON(sbh->b_size != F2FS_BLKSIZE);
> > set_buffer_uptodate(bh);
> > set_buffer_dirty(bh);
> >@@ -1124,6 +1130,10 @@ int f2fs_commit_super(struct f2fs_sb_info *sbi, bool
> >recover)
> >
> > /* write current valid superblock */
> > lock_buffer(sbh);
> >+ if (memcmp(sbh->b_data + F2FS_SUPER_OFFSET, super, sizeof(*super))) {
> >+ f2fs_msg(sbi->sb, KERN_INFO, "Write modified valid superblock");
> >+ memcpy(sbh->b_data + F2FS_SUPER_OFFSET, super, sizeof(*super));
> >+ }
> > set_buffer_dirty(sbh);
> > unlock_buffer(sbh);
> >
> >
> It looks good to me, but can we releasing grabbed block buffer
> 'sbi->raw_super_buf'
> as Chao Yu said.
If then, don't we need to recover the polluted superblock?
>
> Thanks.
> Yunlei
------------------------------------------------------------------------------
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
