| Message ID | 20161125023236.80473-1-jaegeuk@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Jaegeuk, Let me try to understand this, in some cases, we can write a checkpoint pack which has wrong cp_ver, like in 1st cp pack which has even version number or 2nd cp pack which has odd version number, so if we load that kind of cp pack during fill_super, we may load wrong summary data from another older cp pack which cp version pointed to, is that right? Thanks, On 2016/11/25 10:32, Jaegeuk Kim wrote: > We don't guarantee cp_addr is fixed by cp_version. > This is to sync with f2fs-tools. > > Cc: stable@vger.kernel.org > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > --- > fs/f2fs/checkpoint.c | 5 +++++ > fs/f2fs/f2fs.h | 14 +++----------- > 2 files changed, 8 insertions(+), 11 deletions(-) > > diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c > index 54cc6a9..bf2f44c 100644 > --- a/fs/f2fs/checkpoint.c > +++ b/fs/f2fs/checkpoint.c > @@ -789,6 +789,11 @@ int get_valid_checkpoint(struct f2fs_sb_info *sbi) > f2fs_put_page(cur_page, 1); > } > done: > + if (cur_page == cp1) > + sbi->cur_cp_pack = 1; > + else > + sbi->cur_cp_pack = 2; > + > f2fs_put_page(cp1, 1); > f2fs_put_page(cp2, 1); > return 0; > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > index 62383d2..e22e7e1 100644 > --- a/fs/f2fs/f2fs.h > +++ b/fs/f2fs/f2fs.h > @@ -792,6 +792,7 @@ struct f2fs_sb_info { > > /* for checkpoint */ > struct f2fs_checkpoint *ckpt; /* raw checkpoint pointer */ > + int cur_cp_pack; /* remain current cp pack */ > spinlock_t cp_lock; /* for flag in ckpt */ > struct inode *meta_inode; /* cache meta blocks */ > struct mutex cp_mutex; /* checkpoint procedure lock */ > @@ -1352,19 +1353,10 @@ static inline void *__bitmap_ptr(struct f2fs_sb_info *sbi, int flag) > > static inline block_t __start_cp_addr(struct f2fs_sb_info *sbi) > { > - block_t start_addr; > - struct f2fs_checkpoint *ckpt = F2FS_CKPT(sbi); > - unsigned long long ckpt_version = cur_cp_version(ckpt); > - > - start_addr = le32_to_cpu(F2FS_RAW_SUPER(sbi)->cp_blkaddr); > + block_t start_addr = le32_to_cpu(F2FS_RAW_SUPER(sbi)->cp_blkaddr); > > - /* > - * odd numbered checkpoint should at cp segment 0 > - * and even segment must be at cp segment 1 > - */ > - if (!(ckpt_version & 1)) > + if (sbi->cur_cp_pack == 2) > start_addr += sbi->blocks_per_seg; > - > return start_addr; > } > > -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/10, Chao Yu wrote: > Hi Jaegeuk, > > Let me try to understand this, in some cases, we can write a checkpoint pack > which has wrong cp_ver, like in 1st cp pack which has even version number or 2nd > cp pack which has odd version number, so if we load that kind of cp pack during > fill_super, we may load wrong summary data from another older cp pack which cp > version pointed to, is that right? Yup. Especially we can use __start_cp_addr() when finding summary blocks. So, I suspect it was able to cause missing summary blocks. Thanks, > > Thanks, > > On 2016/11/25 10:32, Jaegeuk Kim wrote: > > We don't guarantee cp_addr is fixed by cp_version. > > This is to sync with f2fs-tools. > > > > Cc: stable@vger.kernel.org > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > > --- > > fs/f2fs/checkpoint.c | 5 +++++ > > fs/f2fs/f2fs.h | 14 +++----------- > > 2 files changed, 8 insertions(+), 11 deletions(-) > > > > diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c > > index 54cc6a9..bf2f44c 100644 > > --- a/fs/f2fs/checkpoint.c > > +++ b/fs/f2fs/checkpoint.c > > @@ -789,6 +789,11 @@ int get_valid_checkpoint(struct f2fs_sb_info *sbi) > > f2fs_put_page(cur_page, 1); > > } > > done: > > + if (cur_page == cp1) > > + sbi->cur_cp_pack = 1; > > + else > > + sbi->cur_cp_pack = 2; > > + > > f2fs_put_page(cp1, 1); > > f2fs_put_page(cp2, 1); > > return 0; > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > > index 62383d2..e22e7e1 100644 > > --- a/fs/f2fs/f2fs.h > > +++ b/fs/f2fs/f2fs.h > > @@ -792,6 +792,7 @@ struct f2fs_sb_info { > > > > /* for checkpoint */ > > struct f2fs_checkpoint *ckpt; /* raw checkpoint pointer */ > > + int cur_cp_pack; /* remain current cp pack */ > > spinlock_t cp_lock; /* for flag in ckpt */ > > struct inode *meta_inode; /* cache meta blocks */ > > struct mutex cp_mutex; /* checkpoint procedure lock */ > > @@ -1352,19 +1353,10 @@ static inline void *__bitmap_ptr(struct f2fs_sb_info *sbi, int flag) > > > > static inline block_t __start_cp_addr(struct f2fs_sb_info *sbi) > > { > > - block_t start_addr; > > - struct f2fs_checkpoint *ckpt = F2FS_CKPT(sbi); > > - unsigned long long ckpt_version = cur_cp_version(ckpt); > > - > > - start_addr = le32_to_cpu(F2FS_RAW_SUPER(sbi)->cp_blkaddr); > > + block_t start_addr = le32_to_cpu(F2FS_RAW_SUPER(sbi)->cp_blkaddr); > > > > - /* > > - * odd numbered checkpoint should at cp segment 0 > > - * and even segment must be at cp segment 1 > > - */ > > - if (!(ckpt_version & 1)) > > + if (sbi->cur_cp_pack == 2) > > start_addr += sbi->blocks_per_seg; > > - > > return start_addr; > > } > > > > -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c index 54cc6a9..bf2f44c 100644 --- a/fs/f2fs/checkpoint.c +++ b/fs/f2fs/checkpoint.c @@ -789,6 +789,11 @@ int get_valid_checkpoint(struct f2fs_sb_info *sbi) f2fs_put_page(cur_page, 1); } done: + if (cur_page == cp1) + sbi->cur_cp_pack = 1; + else + sbi->cur_cp_pack = 2; + f2fs_put_page(cp1, 1); f2fs_put_page(cp2, 1); return 0; diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h index 62383d2..e22e7e1 100644 --- a/fs/f2fs/f2fs.h +++ b/fs/f2fs/f2fs.h @@ -792,6 +792,7 @@ struct f2fs_sb_info { /* for checkpoint */ struct f2fs_checkpoint *ckpt; /* raw checkpoint pointer */ + int cur_cp_pack; /* remain current cp pack */ spinlock_t cp_lock; /* for flag in ckpt */ struct inode *meta_inode; /* cache meta blocks */ struct mutex cp_mutex; /* checkpoint procedure lock */ @@ -1352,19 +1353,10 @@ static inline void *__bitmap_ptr(struct f2fs_sb_info *sbi, int flag) static inline block_t __start_cp_addr(struct f2fs_sb_info *sbi) { - block_t start_addr; - struct f2fs_checkpoint *ckpt = F2FS_CKPT(sbi); - unsigned long long ckpt_version = cur_cp_version(ckpt); - - start_addr = le32_to_cpu(F2FS_RAW_SUPER(sbi)->cp_blkaddr); + block_t start_addr = le32_to_cpu(F2FS_RAW_SUPER(sbi)->cp_blkaddr); - /* - * odd numbered checkpoint should at cp segment 0 - * and even segment must be at cp segment 1 - */ - if (!(ckpt_version & 1)) + if (sbi->cur_cp_pack == 2) start_addr += sbi->blocks_per_seg; - return start_addr; }
We don't guarantee cp_addr is fixed by cp_version. This is to sync with f2fs-tools. Cc: stable@vger.kernel.org Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> --- fs/f2fs/checkpoint.c | 5 +++++ fs/f2fs/f2fs.h | 14 +++----------- 2 files changed, 8 insertions(+), 11 deletions(-)