Messages in this thread | | | Date | Mon, 30 Dec 2019 16:06:45 -0800 | From | Jaegeuk Kim <> | Subject | Re: [f2fs-dev] [PATCH 1/4] f2fs: convert inline_dir early before starting rename |
| |
On 12/28, Chao Yu wrote: > On 2019/12/19 4:09, Jaegeuk Kim wrote: > > If we hit an error during rename, we'll get two dentries in different > > directories. > > > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > > --- > > fs/f2fs/f2fs.h | 1 + > > fs/f2fs/inline.c | 30 ++++++++++++++++++++++++++++-- > > fs/f2fs/namei.c | 36 +++++++++++++----------------------- > > 3 files changed, 42 insertions(+), 25 deletions(-) > > > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > > index dbc20d33d0e1..8d64525743cb 100644 > > --- a/fs/f2fs/f2fs.h > > +++ b/fs/f2fs/f2fs.h > > @@ -3490,6 +3490,7 @@ void f2fs_truncate_inline_inode(struct inode *inode, > > int f2fs_read_inline_data(struct inode *inode, struct page *page); > > int f2fs_convert_inline_page(struct dnode_of_data *dn, struct page *page); > > int f2fs_convert_inline_inode(struct inode *inode); > > +int f2fs_convert_inline_dir(struct inode *dir); > > int f2fs_write_inline_data(struct inode *inode, struct page *page); > > bool f2fs_recover_inline_data(struct inode *inode, struct page *npage); > > struct f2fs_dir_entry *f2fs_find_in_inline_dir(struct inode *dir, > > diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c > > index 52f85ed07a15..f82c3d9cf333 100644 > > --- a/fs/f2fs/inline.c > > +++ b/fs/f2fs/inline.c > > @@ -530,7 +530,7 @@ static int f2fs_move_rehashed_dirents(struct inode *dir, struct page *ipage, > > return err; > > } > > > > -static int f2fs_convert_inline_dir(struct inode *dir, struct page *ipage, > > +static int do_convert_inline_dir(struct inode *dir, struct page *ipage, > > void *inline_dentry) > > { > > if (!F2FS_I(dir)->i_dir_level) > > @@ -539,6 +539,32 @@ static int f2fs_convert_inline_dir(struct inode *dir, struct page *ipage, > > return f2fs_move_rehashed_dirents(dir, ipage, inline_dentry); > > } > > > > +int f2fs_convert_inline_dir(struct inode *dir) > > +{ > > + struct f2fs_sb_info *sbi = F2FS_I_SB(dir); > > + struct page *ipage; > > + void *inline_dentry = NULL; > > + int err; > > + > > + if (!f2fs_has_inline_dentry(dir)) > > + return 0; > > + > > + f2fs_lock_op(sbi); > > + > > + ipage = f2fs_get_node_page(sbi, dir->i_ino); > > + if (IS_ERR(ipage)) > > + return PTR_ERR(ipage); > > + > > + inline_dentry = inline_data_addr(dir, ipage); > > + > > + err = do_convert_inline_dir(dir, ipage, inline_dentry); > > + if (!err) > > + f2fs_put_page(ipage, 1); > > + > > + f2fs_unlock_op(sbi); > > + return err; > > +} > > + > > int f2fs_add_inline_entry(struct inode *dir, const struct qstr *new_name, > > const struct qstr *orig_name, > > struct inode *inode, nid_t ino, umode_t mode) > > @@ -562,7 +588,7 @@ int f2fs_add_inline_entry(struct inode *dir, const struct qstr *new_name, > > > > bit_pos = f2fs_room_for_filename(d.bitmap, slots, d.max); > > if (bit_pos >= d.max) { > > - err = f2fs_convert_inline_dir(dir, ipage, inline_dentry); > > + err = do_convert_inline_dir(dir, ipage, inline_dentry); > > if (err) > > return err; > > err = -EAGAIN; > > diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c > > index 5d9584281935..61615ab466c2 100644 > > --- a/fs/f2fs/namei.c > > +++ b/fs/f2fs/namei.c > > @@ -855,7 +855,6 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry, > > struct f2fs_dir_entry *old_dir_entry = NULL; > > struct f2fs_dir_entry *old_entry; > > struct f2fs_dir_entry *new_entry; > > - bool is_old_inline = f2fs_has_inline_dentry(old_dir); > > int err; > > > > if (unlikely(f2fs_cp_error(sbi))) > > @@ -868,6 +867,19 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry, > > F2FS_I(old_dentry->d_inode)->i_projid))) > > return -EXDEV; > > > > + /* > > + * old entry and new entry can locate in the same inline > > + * dentry in inode, when attaching new entry in inline dentry, > > + * it could force inline dentry conversion, after that, > > + * old_entry and old_page will point to wrong address, in > > + * order to avoid this, let's do the check and update here. > > + */ > > The comment is out-of-update here... > > If there is enough room for the new dentry in old directory's inline space, > how about just keeping inline dir state for old directory?
It'd work, but how can we deal with race conditions?
> > > --- > fs/f2fs/dir.c | 14 ++++++++++++++ > fs/f2fs/f2fs.h | 4 +++- > fs/f2fs/inline.c | 22 +++++++++++++++++----- > fs/f2fs/namei.c | 2 +- > 4 files changed, 35 insertions(+), 7 deletions(-) > > diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c > index c967cacf979e..b56f6060c1a6 100644 > --- a/fs/f2fs/dir.c > +++ b/fs/f2fs/dir.c > @@ -578,6 +578,20 @@ int f2fs_room_for_filename(const void *bitmap, int slots, int max_slots) > goto next; > } > > +bool f2fs_has_enough_room(struct inode *dir, struct page *ipage, > + struct fscrypt_name *fname) > +{ > + struct f2fs_dentry_ptr d; > + unsigned int bit_pos; > + int slots = GET_DENTRY_SLOTS(fname_len(fname)); > + > + make_dentry_ptr_inline(dir, &d, inline_data_addr(dir, ipage)); > + > + bit_pos = f2fs_room_for_filename(d.bitmap, slots, d.max); > + > + return bit_pos < d.max; > +} > + > void f2fs_update_dentry(nid_t ino, umode_t mode, struct f2fs_dentry_ptr *d, > const struct qstr *name, f2fs_hash_t name_hash, > unsigned int bit_pos) > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > index 5d55cef66410..ce4ce33a40ce 100644 > --- a/fs/f2fs/f2fs.h > +++ b/fs/f2fs/f2fs.h > @@ -3120,6 +3120,8 @@ ino_t f2fs_inode_by_name(struct inode *dir, const struct qstr *qstr, > struct page **page); > void f2fs_set_link(struct inode *dir, struct f2fs_dir_entry *de, > struct page *page, struct inode *inode); > +bool f2fs_has_enough_room(struct inode *dir, struct page *ipage, > + struct fscrypt_name *fname); > void f2fs_update_dentry(nid_t ino, umode_t mode, struct f2fs_dentry_ptr *d, > const struct qstr *name, f2fs_hash_t name_hash, > unsigned int bit_pos); > @@ -3662,7 +3664,7 @@ void f2fs_truncate_inline_inode(struct inode *inode, > int f2fs_read_inline_data(struct inode *inode, struct page *page); > int f2fs_convert_inline_page(struct dnode_of_data *dn, struct page *page); > int f2fs_convert_inline_inode(struct inode *inode); > -int f2fs_convert_inline_dir(struct inode *dir); > +int f2fs_try_convert_inline_dir(struct inode *dir, struct dentry *dentry); > int f2fs_write_inline_data(struct inode *inode, struct page *page); > bool f2fs_recover_inline_data(struct inode *inode, struct page *npage); > struct f2fs_dir_entry *f2fs_find_in_inline_dir(struct inode *dir, > diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c > index f82c3d9cf333..4167e5408151 100644 > --- a/fs/f2fs/inline.c > +++ b/fs/f2fs/inline.c > @@ -539,28 +539,40 @@ static int do_convert_inline_dir(struct inode *dir, struct page *ipage, > return f2fs_move_rehashed_dirents(dir, ipage, inline_dentry); > } > > -int f2fs_convert_inline_dir(struct inode *dir) > +int f2fs_try_convert_inline_dir(struct inode *dir, struct dentry *dentry) > { > struct f2fs_sb_info *sbi = F2FS_I_SB(dir); > struct page *ipage; > + struct fscrypt_name fname; > void *inline_dentry = NULL; > - int err; > + int err = 0; > > if (!f2fs_has_inline_dentry(dir)) > return 0; > > f2fs_lock_op(sbi); > > + err = fscrypt_setup_filename(dir, &dentry->d_name, 0, &fname); > + if (err) > + goto out; > + > ipage = f2fs_get_node_page(sbi, dir->i_ino); > - if (IS_ERR(ipage)) > - return PTR_ERR(ipage); > + if (IS_ERR(ipage)) { > + err = PTR_ERR(ipage); > + goto out; > + } > + > + if (f2fs_has_enough_room(dir, ipage, &fname)) { > + f2fs_put_page(ipage, 1); > + goto out; > + } > > inline_dentry = inline_data_addr(dir, ipage); > > err = do_convert_inline_dir(dir, ipage, inline_dentry); > if (!err) > f2fs_put_page(ipage, 1); > - > +out: > f2fs_unlock_op(sbi); > return err; > } > diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c > index 1daa54c8d8d8..856f0f984549 100644 > --- a/fs/f2fs/namei.c > +++ b/fs/f2fs/namei.c > @@ -927,7 +927,7 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry, > * order to avoid this, let's do the check and update here. > */ > if (old_dir == new_dir && !new_inode) { > - err = f2fs_convert_inline_dir(old_dir); > + err = f2fs_try_convert_inline_dir(old_dir, new_dentry); > if (err) > return err; > } > -- > 2.18.0.rc1 > > Thanks, > > > + if (old_dir == new_dir && !new_inode) { > > + err = f2fs_convert_inline_dir(old_dir); > > + if (err) > > + return err; > > + } > > + > > if (flags & RENAME_WHITEOUT) { > > err = f2fs_create_whiteout(old_dir, &whiteout); > > if (err) > > @@ -954,28 +966,6 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry, > > > > if (old_dir_entry) > > f2fs_i_links_write(new_dir, true); > > - > > - /* > > - * old entry and new entry can locate in the same inline > > - * dentry in inode, when attaching new entry in inline dentry, > > - * it could force inline dentry conversion, after that, > > - * old_entry and old_page will point to wrong address, in > > - * order to avoid this, let's do the check and update here. > > - */ > > - if (is_old_inline && !f2fs_has_inline_dentry(old_dir)) { > > - f2fs_put_page(old_page, 0); > > - old_page = NULL; > > - > > - old_entry = f2fs_find_entry(old_dir, > > - &old_dentry->d_name, &old_page); > > - if (!old_entry) { > > - err = -ENOENT; > > - if (IS_ERR(old_page)) > > - err = PTR_ERR(old_page); > > - f2fs_unlock_op(sbi); > > - goto out_dir; > > - } > > - } > > } > > > > down_write(&F2FS_I(old_inode)->i_sem); > >
| |