* Re: [PATCH] fs/ntfs3: Rework file operations [not found] <c076bd59-64b1-149b-3a23-5e89a77b7795@paragon-software.com> @ 2021-08-31 21:54 ` Kari Argillander 2021-09-01 16:54 ` Konstantin Komarov 0 siblings, 1 reply; 3+ messages in thread From: Kari Argillander @ 2021-08-31 21:54 UTC (permalink / raw) To: Konstantin Komarov; +Cc: ntfs3, linux-kernel, sfr, linux-fsdevel First of this commit is already in ntfs3/master without review. Please do not do that. We are middle of merge window and new this kind of patch is in. This patch does also lot of refactoring, dead code removel, maybe some bug fixes and who knows what else. One patch should address only one thing. I did not review whole thing but some comments there. I also feel like this is kinda impossible to review because so much happening. This is over 2000 lines long patch. Do you want to review this kind of patches yourself? I cc also fsdevel, maybe someone can help us little bit. Partial review starts here: sfr@canb.auug.org.au ^^ There is this same cc. Accident? Subject: [PATCH] fs/ntfs3: Rework file operations ^^ Not very informative commit header Maybe better should be: Change rename logic to add new name and then remove old one Please also use at least checkpatch. It found three warnings with this patch. On Tue, Aug 31, 2021 at 07:34:28PM +0300, Konstantin Komarov wrote: > Rename now works "Add new name and remove old name". > "Remove old name and add new name" may result in bad inode > if we can't add new name and then can't restore (add) old name. Also little hard to read commit message. Let me help a little bit. Maybe something like (not checked your patch yet): Current rename logic is that we first remove old name and then add new name. This can result bad inode if there is situation that when we add new name and it fails we have no option to return old one. Change behavier so that we first try to add new name and only after that we delete old one. > > Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com> > --- > fs/ntfs3/attrib.c | 17 +-- > fs/ntfs3/attrlist.c | 26 ++-- > fs/ntfs3/frecord.c | 342 ++++++++++++++++++++++++++++++++++++-------- > fs/ntfs3/fsntfs.c | 87 +++++------ > fs/ntfs3/index.c | 45 +++--- > fs/ntfs3/inode.c | 275 ++++++++++++----------------------- > fs/ntfs3/namei.c | 236 ++++++++---------------------- > fs/ntfs3/ntfs_fs.h | 31 +++- > fs/ntfs3/record.c | 8 +- > fs/ntfs3/xattr.c | 25 ++-- > 10 files changed, 563 insertions(+), 529 deletions(-) > > diff --git a/fs/ntfs3/attrib.c b/fs/ntfs3/attrib.c > index 4b285f704e62..ffc323bacc9f 100644 > --- a/fs/ntfs3/attrib.c > +++ b/fs/ntfs3/attrib.c > @@ -218,9 +218,11 @@ int attr_allocate_clusters(struct ntfs_sb_info *sbi, struct runs_tree *run, > } > > out: > - /* Undo. */ > - run_deallocate_ex(sbi, run, vcn0, vcn - vcn0, NULL, false); > - run_truncate(run, vcn0); > + /* Undo 'ntfs_look_for_free_space' */ > + if (vcn - vcn0) { > + run_deallocate_ex(sbi, run, vcn0, vcn - vcn0, NULL, false); > + run_truncate(run, vcn0); > + } Can this happend for both goto out paths? If not then other should return err straight away. > > return err; > } > @@ -701,7 +703,7 @@ int attr_set_size(struct ntfs_inode *ni, enum ATTR_TYPE type, > * (list entry for std attribute always first). > * So it is safe to step back. > */ > - mi_remove_attr(mi, attr); > + mi_remove_attr(NULL, mi, attr); > > if (!al_remove_le(ni, le)) { > err = -EINVAL; > @@ -1004,7 +1006,7 @@ int attr_data_get_block(struct ntfs_inode *ni, CLST vcn, CLST clen, CLST *lcn, > end = next_svcn; > while (end > evcn) { > /* Remove segment [svcn : evcn). */ > - mi_remove_attr(mi, attr); > + mi_remove_attr(NULL, mi, attr); > > if (!al_remove_le(ni, le)) { > err = -EINVAL; > @@ -1600,7 +1602,7 @@ int attr_allocate_frame(struct ntfs_inode *ni, CLST frame, size_t compr_size, > end = next_svcn; > while (end > evcn) { > /* Remove segment [svcn : evcn). */ > - mi_remove_attr(mi, attr); > + mi_remove_attr(NULL, mi, attr); > > if (!al_remove_le(ni, le)) { > err = -EINVAL; > @@ -1836,13 +1838,12 @@ int attr_collapse_range(struct ntfs_inode *ni, u64 vbo, u64 bytes) > u16 le_sz; > u16 roff = le16_to_cpu(attr->nres.run_off); > > - /* run==1 means unpack and deallocate. */ What's whit this one? Old comment maybe but should not be addressed with this patch. > run_unpack_ex(RUN_DEALLOCATE, sbi, ni->mi.rno, svcn, > evcn1 - 1, svcn, Add2Ptr(attr, roff), > le32_to_cpu(attr->size) - roff); > > /* Delete this attribute segment. */ > - mi_remove_attr(mi, attr); > + mi_remove_attr(NULL, mi, attr); > if (!le) > break; > > diff --git a/fs/ntfs3/attrlist.c b/fs/ntfs3/attrlist.c > index 32ca990af64b..fa32399eb517 100644 > --- a/fs/ntfs3/attrlist.c > +++ b/fs/ntfs3/attrlist.c > @@ -279,7 +279,7 @@ int al_add_le(struct ntfs_inode *ni, enum ATTR_TYPE type, const __le16 *name, > struct ATTR_LIST_ENTRY *le; > size_t off; > u16 sz; > - size_t asize, new_asize; > + size_t asize, new_asize, old_size; > u64 new_size; > typeof(ni->attr_list) *al = &ni->attr_list; > > @@ -287,8 +287,9 @@ int al_add_le(struct ntfs_inode *ni, enum ATTR_TYPE type, const __le16 *name, > * Compute the size of the new 'le' > */ > sz = le_size(name_len); > - new_size = al->size + sz; > - asize = al_aligned(al->size); > + old_size = al->size; > + asize = al_aligned(old_size); > new_asize = al_aligned(new_size); > > /* Scan forward to the point at which the new 'le' should be inserted. */ > @@ -302,13 +303,14 @@ int al_add_le(struct ntfs_inode *ni, enum ATTR_TYPE type, const __le16 *name, > return -ENOMEM; > > memcpy(ptr, al->le, off); > - memcpy(Add2Ptr(ptr, off + sz), le, al->size - off); > + memcpy(Add2Ptr(ptr, off + sz), le, old_size - off); > le = Add2Ptr(ptr, off); > kfree(al->le); > al->le = ptr; > } else { > - memmove(Add2Ptr(le, sz), le, al->size - off); > + memmove(Add2Ptr(le, sz), le, old_size - off); > } > + *new_le = le; > > al->size = new_size; > > @@ -321,23 +323,25 @@ int al_add_le(struct ntfs_inode *ni, enum ATTR_TYPE type, const __le16 *name, > le->id = id; > memcpy(le->name, name, sizeof(short) * name_len); > > - al->dirty = true; > - > err = attr_set_size(ni, ATTR_LIST, NULL, 0, &al->run, new_size, > &new_size, true, &attr); > - if (err) > + if (err) { > + /* Undo memmove above. */ There is path also for memcpy. What if we go there. Is this path then impossible? Just checking. > + memmove(le, Add2Ptr(le, sz), old_size - off); > + al->size = old_size; > return err; > + } > + > + al->dirty = true; > > if (attr && attr->non_res) { > err = ntfs_sb_write_run(ni->mi.sbi, &al->run, 0, al->le, > al->size); > if (err) > return err; > + al->dirty = false; > } > > - al->dirty = false; > - *new_le = le; > - > return 0; > } > > diff --git a/fs/ntfs3/frecord.c b/fs/ntfs3/frecord.c > index 9d374827750b..3f48b612ec96 100644 > --- a/fs/ntfs3/frecord.c > +++ b/fs/ntfs3/frecord.c > @@ -163,7 +163,7 @@ int ni_load_mi_ex(struct ntfs_inode *ni, CLST rno, struct mft_inode **mi) > /* > * ni_load_mi - Load mft_inode corresponded list_entry. > */ > -int ni_load_mi(struct ntfs_inode *ni, struct ATTR_LIST_ENTRY *le, > +int ni_load_mi(struct ntfs_inode *ni, const struct ATTR_LIST_ENTRY *le, > struct mft_inode **mi) Feels like unnecesary change for this patch. > { > CLST rno; > @@ -402,7 +402,7 @@ int ni_remove_attr(struct ntfs_inode *ni, enum ATTR_TYPE type, > if (!attr) > return -ENOENT; > > - mi_remove_attr(&ni->mi, attr); > + mi_remove_attr(ni, &ni->mi, attr); > return 0; > } > > @@ -441,7 +441,7 @@ int ni_remove_attr(struct ntfs_inode *ni, enum ATTR_TYPE type, > if (!attr) > return -ENOENT; > > - mi_remove_attr(mi, attr); > + mi_remove_attr(ni, mi, attr); > > if (PtrOffset(ni->attr_list.le, le) >= ni->attr_list.size) > return 0; > @@ -454,12 +454,11 @@ int ni_remove_attr(struct ntfs_inode *ni, enum ATTR_TYPE type, > * > * Return: Not full constructed attribute or NULL if not possible to create. > */ > -static struct ATTRIB *ni_ins_new_attr(struct ntfs_inode *ni, > - struct mft_inode *mi, > - struct ATTR_LIST_ENTRY *le, > - enum ATTR_TYPE type, const __le16 *name, > - u8 name_len, u32 asize, u16 name_off, > - CLST svcn) > +static struct ATTRIB * > +ni_ins_new_attr(struct ntfs_inode *ni, struct mft_inode *mi, > + struct ATTR_LIST_ENTRY *le, enum ATTR_TYPE type, > + const __le16 *name, u8 name_len, u32 asize, u16 name_off, > + CLST svcn, struct ATTR_LIST_ENTRY **ins_le) Reviewing would be lot lot easier if you make one patch which just adds this new ins_le to every function needed. Just call it with NULL. Then there would be much less noise to watch. > { > int err; > struct ATTRIB *attr; > @@ -507,6 +506,8 @@ static struct ATTRIB *ni_ins_new_attr(struct ntfs_inode *ni, > le->ref = ref; > > out: > + if (ins_le) > + *ins_le = le; > return attr; > } > > @@ -599,7 +600,7 @@ static int ni_repack(struct ntfs_inode *ni) > if (next_svcn >= evcn + 1) { > /* We can remove this attribute segment. */ > al_remove_le(ni, le); > - mi_remove_attr(mi, attr); > + mi_remove_attr(NULL, mi, attr); > le = le_p; > continue; > } > @@ -695,8 +696,8 @@ static int ni_try_remove_attr_list(struct ntfs_inode *ni) > free -= asize; > } > > - /* Is seems that attribute list can be removed from primary record. */ > - mi_remove_attr(&ni->mi, attr_list); > + /* It seems that attribute list can be removed from primary record. */ > + mi_remove_attr(NULL, &ni->mi, attr_list); > > /* > * Repeat the cycle above and move all attributes to primary record. > @@ -724,7 +725,7 @@ static int ni_try_remove_attr_list(struct ntfs_inode *ni) > attr_ins->id = id; > > /* Remove from original record. */ > - mi_remove_attr(mi, attr); > + mi_remove_attr(NULL, mi, attr); > } > > run_deallocate(sbi, &ni->attr_list.run, true); > @@ -842,7 +843,8 @@ int ni_create_attr_list(struct ntfs_inode *ni) > memcpy(attr, b, asize); > attr->id = le_b[nb]->id; > > - WARN_ON(!mi_remove_attr(&ni->mi, b)); > + /* Remove from primary record. */ > + WARN_ON(!mi_remove_attr(NULL, &ni->mi, b)); > > if (to_free <= asize) > break; > @@ -883,7 +885,8 @@ int ni_create_attr_list(struct ntfs_inode *ni) > static int ni_ins_attr_ext(struct ntfs_inode *ni, struct ATTR_LIST_ENTRY *le, > enum ATTR_TYPE type, const __le16 *name, u8 name_len, > u32 asize, CLST svcn, u16 name_off, bool force_ext, > - struct ATTRIB **ins_attr, struct mft_inode **ins_mi) > + struct ATTRIB **ins_attr, struct mft_inode **ins_mi, > + struct ATTR_LIST_ENTRY **ins_le) > { > struct ATTRIB *attr; > struct mft_inode *mi; > @@ -956,12 +959,14 @@ static int ni_ins_attr_ext(struct ntfs_inode *ni, struct ATTR_LIST_ENTRY *le, > > /* Try to insert attribute into this subrecord. */ > attr = ni_ins_new_attr(ni, mi, le, type, name, name_len, asize, > - name_off, svcn); > + name_off, svcn, ins_le); > if (!attr) > continue; > > if (ins_attr) > *ins_attr = attr; > + if (ins_mi) > + *ins_mi = mi; > return 0; > } > > @@ -977,7 +982,7 @@ static int ni_ins_attr_ext(struct ntfs_inode *ni, struct ATTR_LIST_ENTRY *le, > } > > attr = ni_ins_new_attr(ni, mi, le, type, name, name_len, asize, > - name_off, svcn); > + name_off, svcn, ins_le); > if (!attr) > goto out2; > > @@ -1016,7 +1021,8 @@ static int ni_ins_attr_ext(struct ntfs_inode *ni, struct ATTR_LIST_ENTRY *le, > static int ni_insert_attr(struct ntfs_inode *ni, enum ATTR_TYPE type, > const __le16 *name, u8 name_len, u32 asize, > u16 name_off, CLST svcn, struct ATTRIB **ins_attr, > - struct mft_inode **ins_mi) > + struct mft_inode **ins_mi, > + struct ATTR_LIST_ENTRY **ins_le) > { > struct ntfs_sb_info *sbi = ni->mi.sbi; > int err; > @@ -1045,7 +1051,7 @@ static int ni_insert_attr(struct ntfs_inode *ni, enum ATTR_TYPE type, > > if (asize <= free) { > attr = ni_ins_new_attr(ni, &ni->mi, NULL, type, name, name_len, > - asize, name_off, svcn); > + asize, name_off, svcn, ins_le); > if (attr) { > if (ins_attr) > *ins_attr = attr; > @@ -1059,7 +1065,8 @@ static int ni_insert_attr(struct ntfs_inode *ni, enum ATTR_TYPE type, > if (!is_mft || type != ATTR_DATA || svcn) { > /* This ATTRIB will be external. */ > err = ni_ins_attr_ext(ni, NULL, type, name, name_len, asize, > - svcn, name_off, false, ins_attr, ins_mi); > + svcn, name_off, false, ins_attr, ins_mi, > + ins_le); > goto out; > } > > @@ -1117,7 +1124,7 @@ static int ni_insert_attr(struct ntfs_inode *ni, enum ATTR_TYPE type, > t16 = le16_to_cpu(attr->name_off); > err = ni_ins_attr_ext(ni, le, attr->type, Add2Ptr(attr, t16), > attr->name_len, t32, attr_svcn(attr), t16, > - false, &eattr, NULL); > + false, &eattr, NULL, NULL); > if (err) > return err; > > @@ -1125,8 +1132,8 @@ static int ni_insert_attr(struct ntfs_inode *ni, enum ATTR_TYPE type, > memcpy(eattr, attr, t32); > eattr->id = id; > > - /* Remove attrib from primary record. */ > - mi_remove_attr(&ni->mi, attr); > + /* Remove from primary record. */ > + mi_remove_attr(NULL, &ni->mi, attr); > > /* attr now points to next attribute. */ > if (attr->type == ATTR_END) > @@ -1136,7 +1143,7 @@ static int ni_insert_attr(struct ntfs_inode *ni, enum ATTR_TYPE type, > ; > > attr = ni_ins_new_attr(ni, &ni->mi, NULL, type, name, name_len, asize, > - name_off, svcn); > + name_off, svcn, ins_le); > if (!attr) { > err = -EINVAL; > goto out; > @@ -1251,7 +1258,7 @@ static int ni_expand_mft_list(struct ntfs_inode *ni) > */ > attr = ni_ins_new_attr(ni, mi_min, NULL, ATTR_DATA, NULL, 0, > SIZEOF_NONRESIDENT + run_size, > - SIZEOF_NONRESIDENT, svcn); > + SIZEOF_NONRESIDENT, svcn, NULL); > if (!attr) { > err = -EINVAL; > goto out; > @@ -1315,14 +1322,15 @@ int ni_expand_list(struct ntfs_inode *ni) > err = ni_ins_attr_ext(ni, le, attr->type, attr_name(attr), > attr->name_len, asize, attr_svcn(attr), > le16_to_cpu(attr->name_off), true, > - &ins_attr, NULL); > + &ins_attr, NULL, NULL); > > if (err) > goto out; > > memcpy(ins_attr, attr, asize); > ins_attr->id = le->id; > - mi_remove_attr(&ni->mi, attr); > + /* Remove from primary record. */ > + mi_remove_attr(NULL, &ni->mi, attr); > > done += asize; > goto out; > @@ -1382,7 +1390,7 @@ int ni_insert_nonresident(struct ntfs_inode *ni, enum ATTR_TYPE type, > } > > err = ni_insert_attr(ni, type, name, name_len, asize, name_off, svcn, > - &attr, mi); > + &attr, mi, NULL); > > if (err) > goto out; > @@ -1422,7 +1430,8 @@ int ni_insert_nonresident(struct ntfs_inode *ni, enum ATTR_TYPE type, > */ > int ni_insert_resident(struct ntfs_inode *ni, u32 data_size, > enum ATTR_TYPE type, const __le16 *name, u8 name_len, > - struct ATTRIB **new_attr, struct mft_inode **mi) > + struct ATTRIB **new_attr, struct mft_inode **mi, > + struct ATTR_LIST_ENTRY **le) > { > int err; > u32 name_size = ALIGN(name_len * sizeof(short), 8); > @@ -1430,7 +1439,7 @@ int ni_insert_resident(struct ntfs_inode *ni, u32 data_size, > struct ATTRIB *attr; > > err = ni_insert_attr(ni, type, name, name_len, asize, SIZEOF_RESIDENT, > - 0, &attr, mi); > + 0, &attr, mi, le); > if (err) > return err; > > @@ -1439,8 +1448,13 @@ int ni_insert_resident(struct ntfs_inode *ni, u32 data_size, > > attr->res.data_size = cpu_to_le32(data_size); > attr->res.data_off = cpu_to_le16(SIZEOF_RESIDENT + name_size); > - if (type == ATTR_NAME) > + if (type == ATTR_NAME) { > attr->res.flags = RESIDENT_FLAG_INDEXED; > + > + /* is_attr_indexed(attr)) == true */ > + le16_add_cpu(&ni->mi.mrec->hard_links, +1); > + ni->mi.dirty = true; > + } > attr->res.res = 0; > > if (new_attr) > @@ -1452,22 +1466,13 @@ int ni_insert_resident(struct ntfs_inode *ni, u32 data_size, > /* > * ni_remove_attr_le - Remove attribute from record. > */ > -int ni_remove_attr_le(struct ntfs_inode *ni, struct ATTRIB *attr, > - struct ATTR_LIST_ENTRY *le) > +void ni_remove_attr_le(struct ntfs_inode *ni, struct ATTRIB *attr, > + struct mft_inode *mi, struct ATTR_LIST_ENTRY *le) > { > - int err; > - struct mft_inode *mi; > - > - err = ni_load_mi(ni, le, &mi); > - if (err) > - return err; > - > - mi_remove_attr(mi, attr); > + mi_remove_attr(ni, mi, attr); > > if (le) > al_remove_le(ni, le); > - > - return 0; > } > > /* > @@ -1549,10 +1554,12 @@ int ni_delete_all(struct ntfs_inode *ni) > > /* ni_fname_name > * > - *Return: File name attribute by its value. */ > + * Return: File name attribute by its value. > + */ > struct ATTR_FILE_NAME *ni_fname_name(struct ntfs_inode *ni, > const struct cpu_str *uni, > const struct MFT_REF *home_dir, > + struct mft_inode **mi, > struct ATTR_LIST_ENTRY **le) > { > struct ATTRIB *attr = NULL; > @@ -1562,7 +1569,7 @@ struct ATTR_FILE_NAME *ni_fname_name(struct ntfs_inode *ni, > > /* Enumerate all names. */ > next: > - attr = ni_find_attr(ni, attr, le, ATTR_NAME, NULL, 0, NULL, NULL); > + attr = ni_find_attr(ni, attr, le, ATTR_NAME, NULL, 0, NULL, mi); > if (!attr) > return NULL; > > @@ -1592,6 +1599,7 @@ struct ATTR_FILE_NAME *ni_fname_name(struct ntfs_inode *ni, > * Return: File name attribute with given type. > */ > struct ATTR_FILE_NAME *ni_fname_type(struct ntfs_inode *ni, u8 name_type, > + struct mft_inode **mi, > struct ATTR_LIST_ENTRY **le) > { > struct ATTRIB *attr = NULL; > @@ -1599,10 +1607,12 @@ struct ATTR_FILE_NAME *ni_fname_type(struct ntfs_inode *ni, u8 name_type, > > *le = NULL; > > + if (FILE_NAME_POSIX == name_type) > + return NULL; > + > /* Enumerate all names. */ > for (;;) { > - attr = ni_find_attr(ni, attr, le, ATTR_NAME, NULL, 0, NULL, > - NULL); > + attr = ni_find_attr(ni, attr, le, ATTR_NAME, NULL, 0, NULL, mi); > if (!attr) > return NULL; > > @@ -2788,6 +2798,222 @@ int ni_write_frame(struct ntfs_inode *ni, struct page **pages, > return err; > } > > +/* > + * ni_remove_name - Removes name 'de' from MFT and from directory. > + * 'de2' and 'undo_step' are used to restore MFT/dir, if error occurs. > + */ > +int ni_remove_name(struct ntfs_inode *dir_ni, struct ntfs_inode *ni, > + struct NTFS_DE *de, struct NTFS_DE **de2, int *undo_step) > +{ > + int err; > + struct ntfs_sb_info *sbi = ni->mi.sbi; > + struct ATTR_FILE_NAME *de_name = (struct ATTR_FILE_NAME *)(de + 1); > + struct ATTR_FILE_NAME *fname; > + struct ATTR_LIST_ENTRY *le; > + struct mft_inode *mi; > + u16 de_key_size = le16_to_cpu(de->key_size); > + u8 name_type; > + > + *undo_step = 0; It looks to me that this function can make undo by itself. There is no need for ni_remove_name_undo. This looks very strange way to do this. One other way if really needed is also to use return value, but this pointer undo_step feels just wrong. > + > + /* Find name in record. */ > + mi_get_ref(&dir_ni->mi, &de_name->home); > + > + fname = ni_fname_name(ni, (struct cpu_str *)&de_name->name_len, > + &de_name->home, &mi, &le); > + if (!fname) > + return -ENOENT; > + > + memcpy(&de_name->dup, &fname->dup, sizeof(struct NTFS_DUP_INFO)); > + name_type = paired_name(fname->type); > + > + /* Mark ntfs as dirty. It will be cleared at umount. */ > + ntfs_set_state(sbi, NTFS_DIRTY_DIRTY); > + > + /* Step 1: Remove name from directory. */ > + err = indx_delete_entry(&dir_ni->dir, dir_ni, fname, de_key_size, sbi); > + if (err) > + return err; > + > + /* Step 2: Remove name from MFT. */ > + ni_remove_attr_le(ni, attr_from_name(fname), mi, le); > + > + *undo_step = 2; > + > + /* Get paired name. */ > + fname = ni_fname_type(ni, name_type, &mi, &le); It does not feel right that ni_fname_type does not return anything if name_type is FILE_NAME_POSIX. It can do it right? Why won't we check before ni_fname_type call if name_type == FILE_NAME_POSIX and just exit with return 0 if it is. > + if (fname) { > + u16 de2_key_size = fname_full_size(fname); > + > + *de2 = Add2Ptr(de, 1024); > + (*de2)->key_size = cpu_to_le16(de2_key_size); > + > + memcpy(*de2 + 1, fname, de2_key_size); > + > + /* Step 3: Remove paired name from directory. */ > + err = indx_delete_entry(&dir_ni->dir, dir_ni, fname, > + de2_key_size, sbi); > + if (err) > + return err; > + > + /* Step 4: Remove paired name from MFT. */ > + ni_remove_attr_le(ni, attr_from_name(fname), mi, le); > + > + *undo_step = 4; > + } > + return 0; > +} > + > +/* > + * ni_remove_name_undo - Paired function for ni_remove_name. > + * > + * Return: True if ok > + */ > +bool ni_remove_name_undo(struct ntfs_inode *dir_ni, struct ntfs_inode *ni, > + struct NTFS_DE *de, struct NTFS_DE *de2, int undo_step) > +{ > + struct ntfs_sb_info *sbi = ni->mi.sbi; > + struct ATTRIB *attr; > + u16 de_key_size = de2 ? le16_to_cpu(de2->key_size) : 0; > + > + switch (undo_step) { > + case 4: > + if (ni_insert_resident(ni, de_key_size, ATTR_NAME, NULL, 0, > + &attr, NULL, NULL)) { > + return false; > + } > + memcpy(Add2Ptr(attr, SIZEOF_RESIDENT), de2 + 1, de_key_size); > + > + mi_get_ref(&ni->mi, &de2->ref); > + de2->size = cpu_to_le16(ALIGN(de_key_size, 8) + > + sizeof(struct NTFS_DE)); > + de2->flags = 0; > + de2->res = 0; > + > + if (indx_insert_entry(&dir_ni->dir, dir_ni, de2, sbi, NULL, > + 1)) { > + return false; > + } > + fallthrough; > + > + case 2: > + de_key_size = le16_to_cpu(de->key_size); > + > + if (ni_insert_resident(ni, de_key_size, ATTR_NAME, NULL, 0, > + &attr, NULL, NULL)) { > + return false; > + } > + > + memcpy(Add2Ptr(attr, SIZEOF_RESIDENT), de + 1, de_key_size); > + mi_get_ref(&ni->mi, &de->ref); > + > + if (indx_insert_entry(&dir_ni->dir, dir_ni, de, sbi, NULL, 1)) { > + return false; > + } > + } > + > + return true; > +} > + > +/* > + * ni_add_name - Add new name in MFT and in directory. *to would be better? Also in comments. > + */ > +int ni_add_name(struct ntfs_inode *dir_ni, struct ntfs_inode *ni, > + struct NTFS_DE *de) > +{ > + int err; > + struct ATTRIB *attr; > + struct ATTR_LIST_ENTRY *le; > + struct mft_inode *mi; > + struct ATTR_FILE_NAME *de_name = (struct ATTR_FILE_NAME *)(de + 1); > + u16 de_key_size = le16_to_cpu(de->key_size); > + > + mi_get_ref(&ni->mi, &de->ref); > + mi_get_ref(&dir_ni->mi, &de_name->home); > + > + /* Insert new name in MFT. */ > + err = ni_insert_resident(ni, de_key_size, ATTR_NAME, NULL, 0, &attr, > + &mi, &le); > + if (err) > + return err; > + > + memcpy(Add2Ptr(attr, SIZEOF_RESIDENT), de_name, de_key_size); > + > + /* Insert new name in directory. */ > + err = indx_insert_entry(&dir_ni->dir, dir_ni, de, ni->mi.sbi, NULL, 0); > + if (err) > + ni_remove_attr_le(ni, attr, mi, le); > + > + return err; > +} > + > +/* > + * ni_rename - Remove one name and insert new name. > + */ > +int ni_rename(struct ntfs_inode *dir_ni, struct ntfs_inode *new_dir_ni, > + struct ntfs_inode *ni, struct NTFS_DE *de, struct NTFS_DE *new_de, > + bool *is_bad) > +{ > + int err; > + struct NTFS_DE *de2 = NULL; > + int undo = 0; > + > + /* > + * There are two possible ways to rename: > + * 1) Add new name and remove old name. > + * 2) Remove old name and add new name. > + * > + * In most cases (not all!) adding new name in MFT and in directory can > + * allocate additional cluster(s). > + * Second way may result to bad inode if we can't add new name > + * and then can't restore (add) old name. > + */ > + > + /* > + * Way 1 - Add new + remove old. > + */ Way too big comment for thing this small. > + err = ni_add_name(new_dir_ni, ni, new_de); if (err) return err; > + if (!err) { > + err = ni_remove_name(dir_ni, ni, de, &de2, &undo); > + if (err && ni_remove_name(new_dir_ni, ni, new_de, &de2, &undo)) > + *is_bad = true; Maybe we can return errno or 1 here. is_bad is again imo little unnecessary here. And now that I think this maybe this should be in caller site in ntfs_rename. > + } > + > + /* > + * Way 2 - Remove old + add new. > + */ > + /* > + * err = ni_remove_name(dir_ni, ni, de, &de2, &undo); > + * if (!err) { > + * err = ni_add_name(new_dir_ni, ni, new_de); > + * if (err && !ni_remove_name_undo(dir_ni, ni, de, de2, undo)) > + * *is_bad = true; > + * } > + */ > + > + return err; > +} > + > +/* > + * ni_is_dirty - Return: True if 'ni' requires ni_write_inode. > + */ > +bool ni_is_dirty(struct inode *inode) > +{ > + struct ntfs_inode *ni = ntfs_i(inode); > + struct rb_node *node; > + > + if (ni->mi.dirty || ni->attr_list.dirty || > + (ni->ni_flags & NI_FLAG_UPDATE_PARENT)) No need to save space here. Just another if statment for third one. > + return true; > + > + for (node = rb_first(&ni->mi_tree); node; node = rb_next(node)) { > + if (rb_entry(node, struct mft_inode, node)->dirty) > + return true; > + } > + > + return false; > +} > + > /* > * ni_update_parent > * > @@ -2802,8 +3028,6 @@ static bool ni_update_parent(struct ntfs_inode *ni, struct NTFS_DUP_INFO *dup, > struct ntfs_sb_info *sbi = ni->mi.sbi; > struct super_block *sb = sbi->sb; > bool re_dirty = false; > - bool active = sb->s_flags & SB_ACTIVE; > - bool upd_parent = ni->ni_flags & NI_FLAG_UPDATE_PARENT; > > if (ni->mi.mrec->flags & RECORD_FLAG_DIR) { > dup->fa |= FILE_ATTRIBUTE_DIRECTORY; > @@ -2867,19 +3091,9 @@ static bool ni_update_parent(struct ntfs_inode *ni, struct NTFS_DUP_INFO *dup, > struct ATTR_FILE_NAME *fname; > > fname = resident_data_ex(attr, SIZEOF_ATTRIBUTE_FILENAME); > - if (!fname) > + if (!fname || !memcmp(&fname->dup, dup, sizeof(fname->dup))) > continue; > > - if (memcmp(&fname->dup, dup, sizeof(fname->dup))) { > - memcpy(&fname->dup, dup, sizeof(fname->dup)); > - mi->dirty = true; > - } else if (!upd_parent) { > - continue; > - } > - > - if (!active) > - continue; /* Avoid __wait_on_freeing_inode(inode); */ > - > /* ntfs_iget5 may sleep. */ > dir = ntfs_iget5(sb, &fname->home, NULL); > if (IS_ERR(dir)) { > @@ -2898,6 +3112,8 @@ static bool ni_update_parent(struct ntfs_inode *ni, struct NTFS_DUP_INFO *dup, > } else { > indx_update_dup(dir_ni, sbi, fname, dup, sync); > ni_unlock(dir_ni); > + memcpy(&fname->dup, dup, sizeof(fname->dup)); > + mi->dirty = true; > } > } > iput(dir); > @@ -2969,7 +3185,9 @@ int ni_write_inode(struct inode *inode, int sync, const char *hint) > ni->mi.dirty = true; > > if (!ntfs_is_meta_file(sbi, inode->i_ino) && > - (modified || (ni->ni_flags & NI_FLAG_UPDATE_PARENT))) { > + (modified || (ni->ni_flags & NI_FLAG_UPDATE_PARENT)) > + /* Avoid __wait_on_freeing_inode(inode). */ > + && (sb->s_flags & SB_ACTIVE)) { > dup.cr_time = std->cr_time; > /* Not critical if this function fail. */ > re_dirty = ni_update_parent(ni, &dup, sync); > @@ -3033,7 +3251,7 @@ int ni_write_inode(struct inode *inode, int sync, const char *hint) > return err; > } > > - if (re_dirty && (sb->s_flags & SB_ACTIVE)) > + if (re_dirty) > mark_inode_dirty_sync(inode); > > return 0; > diff --git a/fs/ntfs3/fsntfs.c b/fs/ntfs3/fsntfs.c > index 0edb95ed9717..669249439217 100644 > --- a/fs/ntfs3/fsntfs.c > +++ b/fs/ntfs3/fsntfs.c > @@ -358,29 +358,25 @@ int ntfs_look_for_free_space(struct ntfs_sb_info *sbi, CLST lcn, CLST len, > enum ALLOCATE_OPT opt) > { > int err; > + CLST alen = 0; > struct super_block *sb = sbi->sb; > - size_t a_lcn, zlen, zeroes, zlcn, zlen2, ztrim, new_zlen; > + size_t alcn, zlen, zeroes, zlcn, zlen2, ztrim, new_zlen; > struct wnd_bitmap *wnd = &sbi->used.bitmap; > > down_write_nested(&wnd->rw_lock, BITMAP_MUTEX_CLUSTERS); > if (opt & ALLOCATE_MFT) { > - CLST alen; > - > zlen = wnd_zone_len(wnd); > > if (!zlen) { > err = ntfs_refresh_zone(sbi); > if (err) > goto out; Before we return this error code. Now we return -ENOSPC if this error. > - > zlen = wnd_zone_len(wnd); > + } > > - if (!zlen) { > - ntfs_err(sbi->sb, > - "no free space to extend mft"); > - err = -ENOSPC; > - goto out; > - } > + if (!zlen) { > + ntfs_err(sbi->sb, "no free space to extend mft"); > + goto out; This is just refactoring and should do in different patch. > } > > lcn = wnd_zone_bit(wnd); > @@ -389,14 +385,13 @@ int ntfs_look_for_free_space(struct ntfs_sb_info *sbi, CLST lcn, CLST len, > wnd_zone_set(wnd, lcn + alen, zlen - alen); > > err = wnd_set_used(wnd, lcn, alen); > - if (err) > - goto out; > - > - *new_lcn = lcn; > - *new_len = alen; > - goto ok; > + if (err) { > + up_write(&wnd->rw_lock); > + return err; > + } > + alcn = lcn; > + goto out; > } > - > /* > * 'Cause cluster 0 is always used this value means that we should use > * cached value of 'next_free_lcn' to improve performance. > @@ -407,22 +402,17 @@ int ntfs_look_for_free_space(struct ntfs_sb_info *sbi, CLST lcn, CLST len, > if (lcn >= wnd->nbits) > lcn = 0; > > - *new_len = wnd_find(wnd, len, lcn, BITMAP_FIND_MARK_AS_USED, &a_lcn); > - if (*new_len) { > - *new_lcn = a_lcn; > - goto ok; > - } > + alen = wnd_find(wnd, len, lcn, BITMAP_FIND_MARK_AS_USED, &alcn); > + if (alen) > + goto out; > > /* Try to use clusters from MftZone. */ > zlen = wnd_zone_len(wnd); > zeroes = wnd_zeroes(wnd); > > - /* Check too big request. */ > - if (len > zeroes + zlen) > - goto no_space; > - > - if (zlen <= NTFS_MIN_MFT_ZONE) > - goto no_space; > + /* Check too big request */ > + if (len > zeroes + zlen || zlen <= NTFS_MIN_MFT_ZONE) > + goto out; > > /* How many clusters to cat from zone. */ > zlcn = wnd_zone_bit(wnd); > @@ -439,31 +429,24 @@ int ntfs_look_for_free_space(struct ntfs_sb_info *sbi, CLST lcn, CLST len, > wnd_zone_set(wnd, zlcn, new_zlen); > > /* Allocate continues clusters. */ > - *new_len = > - wnd_find(wnd, len, 0, > - BITMAP_FIND_MARK_AS_USED | BITMAP_FIND_FULL, &a_lcn); > - if (*new_len) { > - *new_lcn = a_lcn; > - goto ok; > - } > - > -no_space: > - up_write(&wnd->rw_lock); > - > - return -ENOSPC; > - > -ok: > - err = 0; > + alen = wnd_find(wnd, len, 0, > + BITMAP_FIND_MARK_AS_USED | BITMAP_FIND_FULL, &alcn); > > - ntfs_unmap_meta(sb, *new_lcn, *new_len); > +out: > + if (alen) { Just use more goto if needed. This is hard to follow when we come here and when not. > + err = 0; > + *new_len = alen; > + *new_lcn = alcn; > > - if (opt & ALLOCATE_MFT) > - goto out; > + ntfs_unmap_meta(sb, alcn, alen); > > - /* Set hint for next requests. */ > - sbi->used.next_free_lcn = *new_lcn + *new_len; > + /* Set hint for next requests. */ > + if (!(opt & ALLOCATE_MFT)) > + sbi->used.next_free_lcn = alcn + alen; > + } else { > + err = -ENOSPC; > + } > > -out: Example here err_up_write: and you can use that for many place > up_write(&wnd->rw_lock); > return err; > } > @@ -2226,7 +2209,7 @@ int ntfs_insert_security(struct ntfs_sb_info *sbi, > sii_e.sec_id = d_security->key.sec_id; > memcpy(&sii_e.sec_hdr, d_security, SIZEOF_SECURITY_HDR); > > - err = indx_insert_entry(indx_sii, ni, &sii_e.de, NULL, NULL); > + err = indx_insert_entry(indx_sii, ni, &sii_e.de, NULL, NULL, 0); > if (err) > goto out; > > @@ -2247,7 +2230,7 @@ int ntfs_insert_security(struct ntfs_sb_info *sbi, > > fnd_clear(fnd_sdh); > err = indx_insert_entry(indx_sdh, ni, &sdh_e.de, (void *)(size_t)1, > - fnd_sdh); > + fnd_sdh, 0); > if (err) > goto out; > > @@ -2385,7 +2368,7 @@ int ntfs_insert_reparse(struct ntfs_sb_info *sbi, __le32 rtag, > > mutex_lock_nested(&ni->ni_lock, NTFS_INODE_MUTEX_REPARSE); > > - err = indx_insert_entry(indx, ni, &re.de, NULL, NULL); > + err = indx_insert_entry(indx, ni, &re.de, NULL, NULL, 0); > > mark_inode_dirty(&ni->vfs_inode); > ni_unlock(ni); > diff --git a/fs/ntfs3/index.c b/fs/ntfs3/index.c > index 70ef59455b72..1224b8e42b3e 100644 > --- a/fs/ntfs3/index.c > +++ b/fs/ntfs3/index.c > @@ -1427,7 +1427,7 @@ static int indx_create_allocate(struct ntfs_index *indx, struct ntfs_inode *ni, > alloc->nres.valid_size = alloc->nres.data_size = cpu_to_le64(data_size); > > err = ni_insert_resident(ni, bitmap_size(1), ATTR_BITMAP, in->name, > - in->name_len, &bitmap, NULL); > + in->name_len, &bitmap, NULL, NULL); > if (err) > goto out2; > > @@ -1443,7 +1443,7 @@ static int indx_create_allocate(struct ntfs_index *indx, struct ntfs_inode *ni, > return 0; > > out2: > - mi_remove_attr(&ni->mi, alloc); > + mi_remove_attr(NULL, &ni->mi, alloc); > > out1: > run_deallocate(sbi, &run, false); > @@ -1529,24 +1529,24 @@ static int indx_add_allocate(struct ntfs_index *indx, struct ntfs_inode *ni, > /* > * indx_insert_into_root - Attempt to insert an entry into the index root. > * > + * @undo - True if we undoing previous remove. > * If necessary, it will twiddle the index b-tree. > */ > static int indx_insert_into_root(struct ntfs_index *indx, struct ntfs_inode *ni, > const struct NTFS_DE *new_de, > struct NTFS_DE *root_de, const void *ctx, > - struct ntfs_fnd *fnd) > + struct ntfs_fnd *fnd, bool undo) > { > int err = 0; > struct NTFS_DE *e, *e0, *re; > struct mft_inode *mi; > struct ATTRIB *attr; > - struct MFT_REC *rec; > struct INDEX_HDR *hdr; > struct indx_node *n; > CLST new_vbn; > __le64 *sub_vbn, t_vbn; > u16 new_de_size; > - u32 hdr_used, hdr_total, asize, used, to_move; > + u32 hdr_used, hdr_total, asize, to_move; > u32 root_size, new_root_size; > struct ntfs_sb_info *sbi; > int ds_root; > @@ -1559,12 +1559,11 @@ static int indx_insert_into_root(struct ntfs_index *indx, struct ntfs_inode *ni, > > /* > * Try easy case: > - * hdr_insert_de will succeed if there's room the root for the new entry. > + * hdr_insert_de will succeed if there's > + * room the root for the new entry. Not relevant for this patch. > */ > hdr = &root->ihdr; > sbi = ni->mi.sbi; > - rec = mi->mrec; > - used = le32_to_cpu(rec->used); > new_de_size = le16_to_cpu(new_de->size); > hdr_used = le32_to_cpu(hdr->used); > hdr_total = le32_to_cpu(hdr->total); > @@ -1573,9 +1572,9 @@ static int indx_insert_into_root(struct ntfs_index *indx, struct ntfs_inode *ni, > > ds_root = new_de_size + hdr_used - hdr_total; > > - if (used + ds_root < sbi->max_bytes_per_attr) { > - /* Make a room for new elements. */ > - mi_resize_attr(mi, attr, ds_root); > + /* If 'undo' is set then reduce requirements. */ > + if ((undo || asize + ds_root < sbi->max_bytes_per_attr) && > + mi_resize_attr(mi, attr, ds_root)) { > hdr->total = cpu_to_le32(hdr_total + ds_root); > e = hdr_insert_de(indx, hdr, new_de, root_de, ctx); > WARN_ON(!e); > @@ -1629,7 +1628,7 @@ static int indx_insert_into_root(struct ntfs_index *indx, struct ntfs_inode *ni, > sizeof(u64); > ds_root = new_root_size - root_size; > > - if (ds_root > 0 && used + ds_root > sbi->max_bytes_per_attr) { > + if (ds_root > 0 && asize + ds_root > sbi->max_bytes_per_attr) { > /* Make root external. */ > err = -EOPNOTSUPP; > goto out_free_re; > @@ -1710,7 +1709,7 @@ static int indx_insert_into_root(struct ntfs_index *indx, struct ntfs_inode *ni, > > put_indx_node(n); > fnd_clear(fnd); > - err = indx_insert_entry(indx, ni, new_de, ctx, fnd); > + err = indx_insert_entry(indx, ni, new_de, ctx, fnd, undo); > goto out_free_root; > } > > @@ -1854,7 +1853,7 @@ indx_insert_into_buffer(struct ntfs_index *indx, struct ntfs_inode *ni, > */ > if (!level) { > /* Insert in root. */ > - err = indx_insert_into_root(indx, ni, up_e, NULL, ctx, fnd); > + err = indx_insert_into_root(indx, ni, up_e, NULL, ctx, fnd, 0); > if (err) > goto out; > } else { > @@ -1876,10 +1875,12 @@ indx_insert_into_buffer(struct ntfs_index *indx, struct ntfs_inode *ni, > > /* > * indx_insert_entry - Insert new entry into index. > + * > + * @undo - True if we undoing previous remove. > */ > int indx_insert_entry(struct ntfs_index *indx, struct ntfs_inode *ni, > const struct NTFS_DE *new_de, const void *ctx, > - struct ntfs_fnd *fnd) > + struct ntfs_fnd *fnd, bool undo) > { > int err; > int diff; > @@ -1925,7 +1926,7 @@ int indx_insert_entry(struct ntfs_index *indx, struct ntfs_inode *ni, > * new entry into it. > */ > err = indx_insert_into_root(indx, ni, new_de, fnd->root_de, ctx, > - fnd); > + fnd, undo); > if (err) > goto out; > } else { > @@ -2302,7 +2303,7 @@ int indx_delete_entry(struct ntfs_index *indx, struct ntfs_inode *ni, > fnd->level - 1, > fnd) > : indx_insert_into_root(indx, ni, re, e, > - ctx, fnd); > + ctx, fnd, 0); > kfree(re); > > if (err) > @@ -2507,7 +2508,7 @@ int indx_delete_entry(struct ntfs_index *indx, struct ntfs_inode *ni, > * Re-insert the entry into the tree. > * Find the spot the tree where we want to insert the new entry. > */ > - err = indx_insert_entry(indx, ni, me, ctx, fnd); > + err = indx_insert_entry(indx, ni, me, ctx, fnd, 0); > kfree(me); > if (err) > goto out; > @@ -2595,10 +2596,8 @@ int indx_update_dup(struct ntfs_inode *ni, struct ntfs_sb_info *sbi, > struct ntfs_index *indx = &ni->dir; > > fnd = fnd_get(); > - if (!fnd) { > - err = -ENOMEM; > - goto out1; > - } > + if (!fnd) > + return -ENOMEM; > > root = indx_get_root(indx, ni, NULL, &mi); > if (!root) { > @@ -2645,7 +2644,5 @@ int indx_update_dup(struct ntfs_inode *ni, struct ntfs_sb_info *sbi, > > out: > fnd_put(fnd); > - > -out1: > return err; > } > diff --git a/fs/ntfs3/inode.c b/fs/ntfs3/inode.c > index b86ec7dd731c..8f72066b3229 100644 > --- a/fs/ntfs3/inode.c > +++ b/fs/ntfs3/inode.c > @@ -399,6 +399,12 @@ static struct inode *ntfs_read_mft(struct inode *inode, > goto out; > } > > + if (names != le16_to_cpu(rec->hard_links)) { > + /* Correct minor error on the fly. Do not mark inode as dirty. */ > + rec->hard_links = cpu_to_le16(names); > + ni->mi.dirty = true; > + } > + > set_nlink(inode, names); > > if (S_ISDIR(mode)) { > @@ -1279,6 +1285,7 @@ struct inode *ntfs_create_inode(struct user_namespace *mnt_userns, > } > inode = &ni->vfs_inode; > inode_init_owner(mnt_userns, inode, dir, mode); > + mode = inode->i_mode; > > inode->i_atime = inode->i_mtime = inode->i_ctime = ni->i_crtime = > current_time(inode); > @@ -1371,6 +1378,7 @@ struct inode *ntfs_create_inode(struct user_namespace *mnt_userns, > attr = Add2Ptr(attr, asize); > } > > + attr->id = cpu_to_le16(aid++); > if (fa & FILE_ATTRIBUTE_DIRECTORY) { > /* > * Regular directory or symlink to directory. > @@ -1381,7 +1389,6 @@ struct inode *ntfs_create_inode(struct user_namespace *mnt_userns, > > attr->type = ATTR_ROOT; > attr->size = cpu_to_le32(asize); > - attr->id = cpu_to_le16(aid++); > > attr->name_len = ARRAY_SIZE(I30_NAME); > attr->name_off = SIZEOF_RESIDENT_LE; > @@ -1412,52 +1419,46 @@ struct inode *ntfs_create_inode(struct user_namespace *mnt_userns, > /* Insert empty ATTR_DATA */ > attr->type = ATTR_DATA; > attr->size = cpu_to_le32(SIZEOF_RESIDENT); > - attr->id = cpu_to_le16(aid++); > attr->name_off = SIZEOF_RESIDENT_LE; > attr->res.data_off = SIZEOF_RESIDENT_LE; > - } else { > + } else if (S_ISREG(mode)) { This should definetly be own patch. Way too much noice. And yes I think this is a good change but if this is in this patch then how do we see what is relevant? > /* > - * Regular file or node. > + * Regular file. Create empty non resident data attribute. > */ > attr->type = ATTR_DATA; > - attr->id = cpu_to_le16(aid++); > - > - if (S_ISREG(mode)) { > - /* Create empty non resident data attribute. */ > - attr->non_res = 1; > - attr->nres.evcn = cpu_to_le64(-1ll); > - if (fa & FILE_ATTRIBUTE_SPARSE_FILE) { > - attr->size = > - cpu_to_le32(SIZEOF_NONRESIDENT_EX + 8); > - attr->name_off = SIZEOF_NONRESIDENT_EX_LE; > - attr->flags = ATTR_FLAG_SPARSED; > - asize = SIZEOF_NONRESIDENT_EX + 8; > - } else if (fa & FILE_ATTRIBUTE_COMPRESSED) { > - attr->size = > - cpu_to_le32(SIZEOF_NONRESIDENT_EX + 8); > - attr->name_off = SIZEOF_NONRESIDENT_EX_LE; > - attr->flags = ATTR_FLAG_COMPRESSED; > - attr->nres.c_unit = COMPRESSION_UNIT; > - asize = SIZEOF_NONRESIDENT_EX + 8; > - } else { > - attr->size = > - cpu_to_le32(SIZEOF_NONRESIDENT + 8); > - attr->name_off = SIZEOF_NONRESIDENT_LE; > - asize = SIZEOF_NONRESIDENT + 8; > - } > - attr->nres.run_off = attr->name_off; > + attr->non_res = 1; > + attr->nres.evcn = cpu_to_le64(-1ll); > + if (fa & FILE_ATTRIBUTE_SPARSE_FILE) { > + attr->size = cpu_to_le32(SIZEOF_NONRESIDENT_EX + 8); > + attr->name_off = SIZEOF_NONRESIDENT_EX_LE; > + attr->flags = ATTR_FLAG_SPARSED; > + asize = SIZEOF_NONRESIDENT_EX + 8; > + } else if (fa & FILE_ATTRIBUTE_COMPRESSED) { > + attr->size = cpu_to_le32(SIZEOF_NONRESIDENT_EX + 8); > + attr->name_off = SIZEOF_NONRESIDENT_EX_LE; > + attr->flags = ATTR_FLAG_COMPRESSED; > + attr->nres.c_unit = COMPRESSION_UNIT; > + asize = SIZEOF_NONRESIDENT_EX + 8; > } else { > - /* Create empty resident data attribute. */ > - attr->size = cpu_to_le32(SIZEOF_RESIDENT); > - attr->name_off = SIZEOF_RESIDENT_LE; > - if (fa & FILE_ATTRIBUTE_SPARSE_FILE) > - attr->flags = ATTR_FLAG_SPARSED; > - else if (fa & FILE_ATTRIBUTE_COMPRESSED) > - attr->flags = ATTR_FLAG_COMPRESSED; > - attr->res.data_off = SIZEOF_RESIDENT_LE; > - asize = SIZEOF_RESIDENT; > - ni->ni_flags |= NI_FLAG_RESIDENT; > + attr->size = cpu_to_le32(SIZEOF_NONRESIDENT + 8); > + attr->name_off = SIZEOF_NONRESIDENT_LE; > + asize = SIZEOF_NONRESIDENT + 8; > } > + attr->nres.run_off = attr->name_off; > + } else { > + /* > + * Node. Create empty resident data attribute. > + */ > + attr->type = ATTR_DATA; > + attr->size = cpu_to_le32(SIZEOF_RESIDENT); > + attr->name_off = SIZEOF_RESIDENT_LE; > + if (fa & FILE_ATTRIBUTE_SPARSE_FILE) > + attr->flags = ATTR_FLAG_SPARSED; > + else if (fa & FILE_ATTRIBUTE_COMPRESSED) > + attr->flags = ATTR_FLAG_COMPRESSED; > + attr->res.data_off = SIZEOF_RESIDENT_LE; > + asize = SIZEOF_RESIDENT; > + ni->ni_flags |= NI_FLAG_RESIDENT; > } > > if (S_ISDIR(mode)) { > @@ -1485,7 +1486,8 @@ struct inode *ntfs_create_inode(struct user_namespace *mnt_userns, > asize = ALIGN(SIZEOF_RESIDENT + nsize, 8); > t16 = PtrOffset(rec, attr); > > - if (asize + t16 + 8 > sbi->record_size) { > + /* 0x78 - the size of EA + EAINFO to store WSL */ > + if (asize + t16 + 0x78 + 8 > sbi->record_size) { > CLST alen; > CLST clst = bytes_to_cluster(sbi, nsize); > > @@ -1545,20 +1547,15 @@ struct inode *ntfs_create_inode(struct user_namespace *mnt_userns, > rec->next_attr_id = cpu_to_le16(aid); > > /* Step 2: Add new name in index. */ > - err = indx_insert_entry(&dir_ni->dir, dir_ni, new_de, sbi, fnd); > + err = indx_insert_entry(&dir_ni->dir, dir_ni, new_de, sbi, fnd, 0); > if (err) > goto out6; > > - /* Update current directory record. */ > - mark_inode_dirty(dir); > - This was also probably just fix it is own? > inode->i_generation = le16_to_cpu(rec->seq); > > dir->i_mtime = dir->i_ctime = inode->i_atime; > > if (S_ISDIR(mode)) { > - if (dir->i_mode & S_ISGID) > - mode |= S_ISGID; This also is not relevant for this patch. Yes dead code but not releted to this one. This kind of stuff takes lot of reviewers time. I do not even feel like i will continue this review. There is just way too much stuff and would definetly nack. But as this already is in ntfs3 master I just don't know what to do. I cc fsdevel so maybe they can help a little bit. Argillander > inode->i_op = &ntfs_dir_inode_operations; > inode->i_fop = &ntfs_dir_operations; > } else if (S_ISLNK(mode)) { > @@ -1601,8 +1598,8 @@ struct inode *ntfs_create_inode(struct user_namespace *mnt_userns, > d_instantiate(dentry, inode); > > ntfs_save_wsl_perm(inode); > - mark_inode_dirty(inode); > mark_inode_dirty(dir); > + mark_inode_dirty(inode); > > /* Normal exit. */ > goto out2; > @@ -1646,61 +1643,36 @@ struct inode *ntfs_create_inode(struct user_namespace *mnt_userns, > int ntfs_link_inode(struct inode *inode, struct dentry *dentry) > { > int err; > - struct inode *dir = d_inode(dentry->d_parent); > - struct ntfs_inode *dir_ni = ntfs_i(dir); > struct ntfs_inode *ni = ntfs_i(inode); > - struct super_block *sb = inode->i_sb; > - struct ntfs_sb_info *sbi = sb->s_fs_info; > - const struct qstr *name = &dentry->d_name; > - struct NTFS_DE *new_de = NULL; > - struct ATTR_FILE_NAME *fname; > - struct ATTRIB *attr; > - u16 key_size; > - struct INDEX_ROOT *dir_root; > - > - dir_root = indx_get_root(&dir_ni->dir, dir_ni, NULL, NULL); > - if (!dir_root) > - return -EINVAL; > + struct ntfs_sb_info *sbi = inode->i_sb->s_fs_info; > + struct NTFS_DE *de; > + struct ATTR_FILE_NAME *de_name; > > /* Allocate PATH_MAX bytes. */ > - new_de = __getname(); > - if (!new_de) > + de = __getname(); > + if (!de) > return -ENOMEM; > > - /* Mark rw ntfs as dirty. It will be cleared at umount. */ > - ntfs_set_state(ni->mi.sbi, NTFS_DIRTY_DIRTY); > - > - /* Insert file name. */ > - err = fill_name_de(sbi, new_de, name, NULL); > - if (err) > - goto out; > - > - key_size = le16_to_cpu(new_de->key_size); > - err = ni_insert_resident(ni, key_size, ATTR_NAME, NULL, 0, &attr, NULL); > - if (err) > - goto out; > - > - mi_get_ref(&ni->mi, &new_de->ref); > - > - fname = (struct ATTR_FILE_NAME *)(new_de + 1); > - mi_get_ref(&dir_ni->mi, &fname->home); > - fname->dup.cr_time = fname->dup.m_time = fname->dup.c_time = > - fname->dup.a_time = kernel2nt(&inode->i_ctime); > - fname->dup.alloc_size = fname->dup.data_size = 0; > - fname->dup.fa = ni->std_fa; > - fname->dup.ea_size = fname->dup.reparse = 0; > - > - memcpy(Add2Ptr(attr, SIZEOF_RESIDENT), fname, key_size); > + /* Mark rw ntfs as dirty. It will be cleared at umount. */ > + ntfs_set_state(sbi, NTFS_DIRTY_DIRTY); > > - err = indx_insert_entry(&dir_ni->dir, dir_ni, new_de, sbi, NULL); > + /* Construct 'de'. */ > + err = fill_name_de(sbi, de, &dentry->d_name, NULL); > if (err) > goto out; > > - le16_add_cpu(&ni->mi.mrec->hard_links, 1); > - ni->mi.dirty = true; > + de_name = (struct ATTR_FILE_NAME *)(de + 1); > + /* Fill duplicate info. */ > + de_name->dup.cr_time = de_name->dup.m_time = de_name->dup.c_time = > + de_name->dup.a_time = kernel2nt(&inode->i_ctime); > + de_name->dup.alloc_size = de_name->dup.data_size = > + cpu_to_le64(inode->i_size); > + de_name->dup.fa = ni->std_fa; > + de_name->dup.ea_size = de_name->dup.reparse = 0; > > + err = ni_add_name(ntfs_i(d_inode(dentry->d_parent)), ni, de); > out: > - __putname(new_de); > + __putname(de); > return err; > } > > @@ -1713,113 +1685,56 @@ int ntfs_link_inode(struct inode *inode, struct dentry *dentry) > int ntfs_unlink_inode(struct inode *dir, const struct dentry *dentry) > { > int err; > - struct super_block *sb = dir->i_sb; > - struct ntfs_sb_info *sbi = sb->s_fs_info; > + struct ntfs_sb_info *sbi = dir->i_sb->s_fs_info; > struct inode *inode = d_inode(dentry); > struct ntfs_inode *ni = ntfs_i(inode); > - const struct qstr *name = &dentry->d_name; > struct ntfs_inode *dir_ni = ntfs_i(dir); > - struct ntfs_index *indx = &dir_ni->dir; > - struct cpu_str *uni = NULL; > - struct ATTR_FILE_NAME *fname; > - u8 name_type; > - struct ATTR_LIST_ENTRY *le; > - struct MFT_REF ref; > - bool is_dir = S_ISDIR(inode->i_mode); > - struct INDEX_ROOT *dir_root; > + struct NTFS_DE *de, *de2 = NULL; > + int undo_remove; > > - dir_root = indx_get_root(indx, dir_ni, NULL, NULL); > - if (!dir_root) > + if (ntfs_is_meta_file(sbi, ni->mi.rno)) > return -EINVAL; > > + /* Allocate PATH_MAX bytes. */ > + de = __getname(); > + if (!de) > + return -ENOMEM; > + > ni_lock(ni); > > - if (is_dir && !dir_is_empty(inode)) { > + if (S_ISDIR(inode->i_mode) && !dir_is_empty(inode)) { > err = -ENOTEMPTY; > - goto out1; > - } > - > - if (ntfs_is_meta_file(sbi, inode->i_ino)) { > - err = -EINVAL; > - goto out1; > - } > - > - /* Allocate PATH_MAX bytes. */ > - uni = __getname(); > - if (!uni) { > - err = -ENOMEM; > - goto out1; > + goto out; > } > > - /* Convert input string to unicode. */ > - err = ntfs_nls_to_utf16(sbi, name->name, name->len, uni, NTFS_NAME_LEN, > - UTF16_HOST_ENDIAN); > + err = fill_name_de(sbi, de, &dentry->d_name, NULL); > if (err < 0) > - goto out2; > - > - /* Mark rw ntfs as dirty. It will be cleared at umount. */ > - ntfs_set_state(sbi, NTFS_DIRTY_DIRTY); > - > - /* Find name in record. */ > - mi_get_ref(&dir_ni->mi, &ref); > - > - le = NULL; > - fname = ni_fname_name(ni, uni, &ref, &le); > - if (!fname) { > - err = -ENOENT; > - goto out3; > - } > - > - name_type = paired_name(fname->type); > - > - err = indx_delete_entry(indx, dir_ni, fname, fname_full_size(fname), > - sbi); > - if (err) > - goto out3; > - > - /* Then remove name from MFT. */ > - ni_remove_attr_le(ni, attr_from_name(fname), le); > - > - le16_add_cpu(&ni->mi.mrec->hard_links, -1); > - ni->mi.dirty = true; > - > - if (name_type != FILE_NAME_POSIX) { > - /* Now we should delete name by type. */ > - fname = ni_fname_type(ni, name_type, &le); > - if (fname) { > - err = indx_delete_entry(indx, dir_ni, fname, > - fname_full_size(fname), sbi); > - if (err) > - goto out3; > + goto out; > > - ni_remove_attr_le(ni, attr_from_name(fname), le); > + undo_remove = 0; > + err = ni_remove_name(dir_ni, ni, de, &de2, &undo_remove); > > - le16_add_cpu(&ni->mi.mrec->hard_links, -1); > - } > - } > -out3: > - switch (err) { > - case 0: > + if (!err) { > drop_nlink(inode); > - break; > - case -ENOTEMPTY: > - case -ENOSPC: > - case -EROFS: > - break; > - default: > + dir->i_mtime = dir->i_ctime = current_time(dir); > + mark_inode_dirty(dir); > + inode->i_ctime = dir->i_ctime; > + if (inode->i_nlink) > + mark_inode_dirty(inode); > + } else if (!ni_remove_name_undo(dir_ni, ni, de, de2, undo_remove)) { > make_bad_inode(inode); > + ntfs_inode_err(inode, "failed to undo unlink"); > + ntfs_set_state(sbi, NTFS_DIRTY_ERROR); > + } else { > + if (ni_is_dirty(dir)) > + mark_inode_dirty(dir); > + if (ni_is_dirty(inode)) > + mark_inode_dirty(inode); > } > > - dir->i_mtime = dir->i_ctime = current_time(dir); > - mark_inode_dirty(dir); > - inode->i_ctime = dir->i_ctime; > - if (inode->i_nlink) > - mark_inode_dirty(inode); > - > -out2: > - __putname(uni); > -out1: > +out: > ni_unlock(ni); > + __putname(de); > return err; > } > > diff --git a/fs/ntfs3/namei.c b/fs/ntfs3/namei.c > index f79a399bd015..e58415d07132 100644 > --- a/fs/ntfs3/namei.c > +++ b/fs/ntfs3/namei.c > @@ -152,12 +152,14 @@ static int ntfs_link(struct dentry *ode, struct inode *dir, struct dentry *de) > if (inode != dir) > ni_lock(ni); > > - dir->i_ctime = dir->i_mtime = inode->i_ctime = current_time(inode); > inc_nlink(inode); > ihold(inode); > > err = ntfs_link_inode(inode, de); > + > if (!err) { > + dir->i_ctime = dir->i_mtime = inode->i_ctime = > + current_time(dir); > mark_inode_dirty(inode); > mark_inode_dirty(dir); > d_instantiate(de, inode); > @@ -249,25 +251,26 @@ static int ntfs_rmdir(struct inode *dir, struct dentry *dentry) > /* > * ntfs_rename - inode_operations::rename > */ > -static int ntfs_rename(struct user_namespace *mnt_userns, struct inode *old_dir, > - struct dentry *old_dentry, struct inode *new_dir, > +static int ntfs_rename(struct user_namespace *mnt_userns, struct inode *dir, > + struct dentry *dentry, struct inode *new_dir, > struct dentry *new_dentry, u32 flags) > { > int err; > - struct super_block *sb = old_dir->i_sb; > + struct super_block *sb = dir->i_sb; > struct ntfs_sb_info *sbi = sb->s_fs_info; > - struct ntfs_inode *old_dir_ni = ntfs_i(old_dir); > + struct ntfs_inode *dir_ni = ntfs_i(dir); > struct ntfs_inode *new_dir_ni = ntfs_i(new_dir); > - struct ntfs_inode *old_ni; > - struct ATTR_FILE_NAME *old_name, *new_name, *fname; > - u8 name_type; > - bool is_same; > - struct inode *old_inode, *new_inode; > - struct NTFS_DE *old_de, *new_de; > - struct ATTRIB *attr; > - struct ATTR_LIST_ENTRY *le; > - u16 new_de_key_size; > - > + struct inode *inode = d_inode(dentry); > + struct ntfs_inode *ni = ntfs_i(inode); > + struct inode *new_inode = d_inode(new_dentry); > + struct NTFS_DE *de, *new_de; > + bool is_same, is_bad; > + /* > + * de - memory of PATH_MAX bytes: > + * [0-1024) - original name (dentry->d_name) > + * [1024-2048) - paired to original name, usually DOS variant of dentry->d_name > + * [2048-3072) - new name (new_dentry->d_name) > + */ > static_assert(SIZEOF_ATTRIBUTE_FILENAME_MAX + SIZEOF_RESIDENT < 1024); > static_assert(SIZEOF_ATTRIBUTE_FILENAME_MAX + sizeof(struct NTFS_DE) < > 1024); > @@ -276,24 +279,18 @@ static int ntfs_rename(struct user_namespace *mnt_userns, struct inode *old_dir, > if (flags & ~RENAME_NOREPLACE) > return -EINVAL; > > - old_inode = d_inode(old_dentry); > - new_inode = d_inode(new_dentry); > - > - old_ni = ntfs_i(old_inode); > + is_same = dentry->d_name.len == new_dentry->d_name.len && > + !memcmp(dentry->d_name.name, new_dentry->d_name.name, > + dentry->d_name.len); > > - is_same = old_dentry->d_name.len == new_dentry->d_name.len && > - !memcmp(old_dentry->d_name.name, new_dentry->d_name.name, > - old_dentry->d_name.len); > - > - if (is_same && old_dir == new_dir) { > + if (is_same && dir == new_dir) { > /* Nothing to do. */ > - err = 0; > - goto out; > + return 0; > } > > - if (ntfs_is_meta_file(sbi, old_inode->i_ino)) { > - err = -EINVAL; > - goto out; > + if (ntfs_is_meta_file(sbi, inode->i_ino)) { > + /* Should we print an error? */ > + return -EINVAL; > } > > if (new_inode) { > @@ -304,168 +301,61 @@ static int ntfs_rename(struct user_namespace *mnt_userns, struct inode *old_dir, > ni_unlock(new_dir_ni); > dput(new_dentry); > if (err) > - goto out; > + return err; > } > > /* Allocate PATH_MAX bytes. */ > - old_de = __getname(); > - if (!old_de) { > - err = -ENOMEM; > - goto out; > - } > + de = __getname(); > + if (!de) > + return -ENOMEM; > > - err = fill_name_de(sbi, old_de, &old_dentry->d_name, NULL); > + /* Translate dentry->d_name into unicode form. */ > + err = fill_name_de(sbi, de, &dentry->d_name, NULL); > if (err < 0) > - goto out1; > - > - old_name = (struct ATTR_FILE_NAME *)(old_de + 1); > + goto out; > > if (is_same) { > - new_de = old_de; > + /* Reuse 'de'. */ > + new_de = de; > } else { > - new_de = Add2Ptr(old_de, 1024); > + /* Translate new_dentry->d_name into unicode form. */ > + new_de = Add2Ptr(de, 2048); > err = fill_name_de(sbi, new_de, &new_dentry->d_name, NULL); > if (err < 0) > - goto out1; > - } > - > - ni_lock_dir(old_dir_ni); > - ni_lock(old_ni); > - > - mi_get_ref(&old_dir_ni->mi, &old_name->home); > - > - /* Get pointer to file_name in MFT. */ > - fname = ni_fname_name(old_ni, (struct cpu_str *)&old_name->name_len, > - &old_name->home, &le); > - if (!fname) { > - err = -EINVAL; > - goto out2; > + goto out; > } > > - /* Copy fname info from record into new fname. */ > - new_name = (struct ATTR_FILE_NAME *)(new_de + 1); > - memcpy(&new_name->dup, &fname->dup, sizeof(fname->dup)); > - > - name_type = paired_name(fname->type); > - > - /* Remove first name from directory. */ > - err = indx_delete_entry(&old_dir_ni->dir, old_dir_ni, old_de + 1, > - le16_to_cpu(old_de->key_size), sbi); > - if (err) > - goto out3; > - > - /* Remove first name from MFT. */ > - err = ni_remove_attr_le(old_ni, attr_from_name(fname), le); > - if (err) > - goto out4; > - > - le16_add_cpu(&old_ni->mi.mrec->hard_links, -1); > - old_ni->mi.dirty = true; > - > - if (name_type != FILE_NAME_POSIX) { > - /* Get paired name. */ > - fname = ni_fname_type(old_ni, name_type, &le); > - if (fname) { > - /* Remove second name from directory. */ > - err = indx_delete_entry(&old_dir_ni->dir, old_dir_ni, > - fname, fname_full_size(fname), > - sbi); > - if (err) > - goto out5; > - > - /* Remove second name from MFT. */ > - err = ni_remove_attr_le(old_ni, attr_from_name(fname), > - le); > - if (err) > - goto out6; > - > - le16_add_cpu(&old_ni->mi.mrec->hard_links, -1); > - old_ni->mi.dirty = true; > + ni_lock_dir(dir_ni); > + ni_lock(ni); > + > + is_bad = false; > + err = ni_rename(dir_ni, new_dir_ni, ni, de, new_de, &is_bad); > + if (is_bad) { > + /* Restore after failed rename failed too. */ > + make_bad_inode(inode); > + ntfs_inode_err(inode, "failed to undo rename"); > + ntfs_set_state(sbi, NTFS_DIRTY_ERROR); > + } else if (!err) { > + inode->i_ctime = dir->i_ctime = dir->i_mtime = > + current_time(dir); > + mark_inode_dirty(inode); > + mark_inode_dirty(dir); > + if (dir != new_dir) { > + new_dir->i_mtime = new_dir->i_ctime = dir->i_ctime; > + mark_inode_dirty(new_dir); > } > - } > - > - /* Add new name. */ > - mi_get_ref(&old_ni->mi, &new_de->ref); > - mi_get_ref(&ntfs_i(new_dir)->mi, &new_name->home); > - > - new_de_key_size = le16_to_cpu(new_de->key_size); > - > - /* Insert new name in MFT. */ > - err = ni_insert_resident(old_ni, new_de_key_size, ATTR_NAME, NULL, 0, > - &attr, NULL); > - if (err) > - goto out7; > - > - attr->res.flags = RESIDENT_FLAG_INDEXED; > - > - memcpy(Add2Ptr(attr, SIZEOF_RESIDENT), new_name, new_de_key_size); > - > - le16_add_cpu(&old_ni->mi.mrec->hard_links, 1); > - old_ni->mi.dirty = true; > - > - /* Insert new name in directory. */ > - err = indx_insert_entry(&new_dir_ni->dir, new_dir_ni, new_de, sbi, > - NULL); > - if (err) > - goto out8; > > - if (IS_DIRSYNC(new_dir)) > - err = ntfs_sync_inode(old_inode); > - else > - mark_inode_dirty(old_inode); > + if (IS_DIRSYNC(dir)) > + ntfs_sync_inode(dir); > > - old_dir->i_ctime = old_dir->i_mtime = current_time(old_dir); > - if (IS_DIRSYNC(old_dir)) > - (void)ntfs_sync_inode(old_dir); > - else > - mark_inode_dirty(old_dir); > - > - if (old_dir != new_dir) { > - new_dir->i_mtime = new_dir->i_ctime = old_dir->i_ctime; > - mark_inode_dirty(new_dir); > - } > - > - if (old_inode) { > - old_inode->i_ctime = old_dir->i_ctime; > - mark_inode_dirty(old_inode); > + if (IS_DIRSYNC(new_dir)) > + ntfs_sync_inode(inode); > } > > - err = 0; > - /* Normal way* */ > - goto out2; > - > -out8: > - /* undo > - * ni_insert_resident(old_ni, new_de_key_size, ATTR_NAME, NULL, 0, > - * &attr, NULL); > - */ > - mi_remove_attr(&old_ni->mi, attr); > -out7: > - /* undo > - * ni_remove_attr_le(old_ni, attr_from_name(fname), le); > - */ > -out6: > - /* undo > - * indx_delete_entry(&old_dir_ni->dir, old_dir_ni, > - * fname, fname_full_size(fname), > - * sbi); > - */ > -out5: > - /* undo > - * ni_remove_attr_le(old_ni, attr_from_name(fname), le); > - */ > -out4: > - /* undo: > - * indx_delete_entry(&old_dir_ni->dir, old_dir_ni, old_de + 1, > - * old_de->key_size, NULL); > - */ > -out3: > -out2: > - ni_unlock(old_ni); > - ni_unlock(old_dir_ni); > -out1: > - __putname(old_de); > + ni_unlock(ni); > + ni_unlock(dir_ni); > out: > + __putname(de); > return err; > } > > diff --git a/fs/ntfs3/ntfs_fs.h b/fs/ntfs3/ntfs_fs.h > index 64ef92e16363..f9436cbbc347 100644 > --- a/fs/ntfs3/ntfs_fs.h > +++ b/fs/ntfs3/ntfs_fs.h > @@ -478,7 +478,7 @@ struct ATTR_STD_INFO *ni_std(struct ntfs_inode *ni); > struct ATTR_STD_INFO5 *ni_std5(struct ntfs_inode *ni); > void ni_clear(struct ntfs_inode *ni); > int ni_load_mi_ex(struct ntfs_inode *ni, CLST rno, struct mft_inode **mi); > -int ni_load_mi(struct ntfs_inode *ni, struct ATTR_LIST_ENTRY *le, > +int ni_load_mi(struct ntfs_inode *ni, const struct ATTR_LIST_ENTRY *le, > struct mft_inode **mi); > struct ATTRIB *ni_find_attr(struct ntfs_inode *ni, struct ATTRIB *attr, > struct ATTR_LIST_ENTRY **entry_o, > @@ -505,15 +505,18 @@ int ni_insert_nonresident(struct ntfs_inode *ni, enum ATTR_TYPE type, > struct mft_inode **mi); > int ni_insert_resident(struct ntfs_inode *ni, u32 data_size, > enum ATTR_TYPE type, const __le16 *name, u8 name_len, > - struct ATTRIB **new_attr, struct mft_inode **mi); > -int ni_remove_attr_le(struct ntfs_inode *ni, struct ATTRIB *attr, > - struct ATTR_LIST_ENTRY *le); > + struct ATTRIB **new_attr, struct mft_inode **mi, > + struct ATTR_LIST_ENTRY **le); > +void ni_remove_attr_le(struct ntfs_inode *ni, struct ATTRIB *attr, > + struct mft_inode *mi, struct ATTR_LIST_ENTRY *le); > int ni_delete_all(struct ntfs_inode *ni); > struct ATTR_FILE_NAME *ni_fname_name(struct ntfs_inode *ni, > const struct cpu_str *uni, > const struct MFT_REF *home, > + struct mft_inode **mi, > struct ATTR_LIST_ENTRY **entry); > struct ATTR_FILE_NAME *ni_fname_type(struct ntfs_inode *ni, u8 name_type, > + struct mft_inode **mi, > struct ATTR_LIST_ENTRY **entry); > int ni_new_attr_flags(struct ntfs_inode *ni, enum FILE_ATTRIBUTE new_fa); > enum REPARSE_SIGN ni_parse_reparse(struct ntfs_inode *ni, struct ATTRIB *attr, > @@ -528,6 +531,21 @@ int ni_read_frame(struct ntfs_inode *ni, u64 frame_vbo, struct page **pages, > u32 pages_per_frame); > int ni_write_frame(struct ntfs_inode *ni, struct page **pages, > u32 pages_per_frame); > +int ni_remove_name(struct ntfs_inode *dir_ni, struct ntfs_inode *ni, > + struct NTFS_DE *de, struct NTFS_DE **de2, int *undo_step); > + > +bool ni_remove_name_undo(struct ntfs_inode *dir_ni, struct ntfs_inode *ni, > + struct NTFS_DE *de, struct NTFS_DE *de2, > + int undo_step); > + > +int ni_add_name(struct ntfs_inode *dir_ni, struct ntfs_inode *ni, > + struct NTFS_DE *de); > + > +int ni_rename(struct ntfs_inode *dir_ni, struct ntfs_inode *new_dir_ni, > + struct ntfs_inode *ni, struct NTFS_DE *de, struct NTFS_DE *new_de, > + bool *is_bad); > + > +bool ni_is_dirty(struct inode *inode); > > /* Globals from fslog.c */ > int log_replay(struct ntfs_inode *ni, bool *initialized); > @@ -631,7 +649,7 @@ int indx_find_raw(struct ntfs_index *indx, struct ntfs_inode *ni, > size_t *off, struct ntfs_fnd *fnd); > int indx_insert_entry(struct ntfs_index *indx, struct ntfs_inode *ni, > const struct NTFS_DE *new_de, const void *param, > - struct ntfs_fnd *fnd); > + struct ntfs_fnd *fnd, bool undo); > int indx_delete_entry(struct ntfs_index *indx, struct ntfs_inode *ni, > const void *key, u32 key_len, const void *param); > int indx_update_dup(struct ntfs_inode *ni, struct ntfs_sb_info *sbi, > @@ -694,7 +712,8 @@ struct ATTRIB *mi_insert_attr(struct mft_inode *mi, enum ATTR_TYPE type, > const __le16 *name, u8 name_len, u32 asize, > u16 name_off); > > -bool mi_remove_attr(struct mft_inode *mi, struct ATTRIB *attr); > +bool mi_remove_attr(struct ntfs_inode *ni, struct mft_inode *mi, > + struct ATTRIB *attr); > bool mi_resize_attr(struct mft_inode *mi, struct ATTRIB *attr, int bytes); > int mi_pack_runs(struct mft_inode *mi, struct ATTRIB *attr, > struct runs_tree *run, CLST len); > diff --git a/fs/ntfs3/record.c b/fs/ntfs3/record.c > index d48a5e6c5045..61e3f2fb619f 100644 > --- a/fs/ntfs3/record.c > +++ b/fs/ntfs3/record.c > @@ -489,7 +489,8 @@ struct ATTRIB *mi_insert_attr(struct mft_inode *mi, enum ATTR_TYPE type, > * > * NOTE: The source attr will point to next attribute. > */ > -bool mi_remove_attr(struct mft_inode *mi, struct ATTRIB *attr) > +bool mi_remove_attr(struct ntfs_inode *ni, struct mft_inode *mi, > + struct ATTRIB *attr) > { > struct MFT_REC *rec = mi->mrec; > u32 aoff = PtrOffset(rec, attr); > @@ -499,6 +500,11 @@ bool mi_remove_attr(struct mft_inode *mi, struct ATTRIB *attr) > if (aoff + asize > used) > return false; > > + if (ni && is_attr_indexed(attr)) { > + le16_add_cpu(&ni->mi.mrec->hard_links, -1); > + ni->mi.dirty = true; > + } > + > used -= asize; > memmove(attr, Add2Ptr(attr, asize), used - aoff); > rec->used = cpu_to_le32(used); > diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c > index b4c921e4bc1a..22fd5eb32c5b 100644 > --- a/fs/ntfs3/xattr.c > +++ b/fs/ntfs3/xattr.c > @@ -395,11 +395,13 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name, > } > > err = ni_insert_resident(ni, sizeof(struct EA_INFO), > - ATTR_EA_INFO, NULL, 0, NULL, NULL); > + ATTR_EA_INFO, NULL, 0, NULL, NULL, > + NULL); > if (err) > goto out; > > - err = ni_insert_resident(ni, 0, ATTR_EA, NULL, 0, NULL, NULL); > + err = ni_insert_resident(ni, 0, ATTR_EA, NULL, 0, NULL, NULL, > + NULL); > if (err) > goto out; > } > @@ -419,9 +421,7 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name, > > if (!size) { > /* Delete xattr, ATTR_EA_INFO */ > - err = ni_remove_attr_le(ni, attr, le); > - if (err) > - goto out; > + ni_remove_attr_le(ni, attr, mi, le); > } else { > p = resident_data_ex(attr, sizeof(struct EA_INFO)); > if (!p) { > @@ -441,9 +441,7 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name, > > if (!size) { > /* Delete xattr, ATTR_EA */ > - err = ni_remove_attr_le(ni, attr, le); > - if (err) > - goto out; > + ni_remove_attr_le(ni, attr, mi, le); > } else if (attr->non_res) { > err = ntfs_sb_write_run(sbi, &ea_run, 0, ea_all, size); > if (err) > @@ -605,8 +603,7 @@ static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns, > goto out; > } > > - err = ntfs_set_ea(inode, name, name_len, value, size, > - acl ? 0 : XATTR_REPLACE, locked); > + err = ntfs_set_ea(inode, name, name_len, value, size, 0, locked); > if (!err) > set_cached_acl(inode, type, acl); > > @@ -632,8 +629,10 @@ static int ntfs_xattr_get_acl(struct user_namespace *mnt_userns, > struct posix_acl *acl; > int err; > > - if (!(inode->i_sb->s_flags & SB_POSIXACL)) > + if (!(inode->i_sb->s_flags & SB_POSIXACL)) { > + ntfs_inode_warn(inode, "add mount option \"acl\" to use acl"); > return -EOPNOTSUPP; > + } > > acl = ntfs_get_acl(inode, type); > if (IS_ERR(acl)) > @@ -655,8 +654,10 @@ static int ntfs_xattr_set_acl(struct user_namespace *mnt_userns, > struct posix_acl *acl; > int err; > > - if (!(inode->i_sb->s_flags & SB_POSIXACL)) > + if (!(inode->i_sb->s_flags & SB_POSIXACL)) { > + ntfs_inode_warn(inode, "add mount option \"acl\" to use acl"); > return -EOPNOTSUPP; > + } > > if (!inode_owner_or_capable(mnt_userns, inode)) > return -EPERM; > -- > 2.28.0 > ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] fs/ntfs3: Rework file operations 2021-08-31 21:54 ` [PATCH] fs/ntfs3: Rework file operations Kari Argillander @ 2021-09-01 16:54 ` Konstantin Komarov 2021-09-02 8:25 ` Kari Argillander 0 siblings, 1 reply; 3+ messages in thread From: Konstantin Komarov @ 2021-09-01 16:54 UTC (permalink / raw) To: Kari Argillander; +Cc: ntfs3, linux-kernel, linux-fsdevel On 01.09.2021 00:54, Kari Argillander wrote: > First of this commit is already in ntfs3/master without review. Please > do not do that. We are middle of merge window and new this kind of patch > is in. > > This patch does also lot of refactoring, dead code removel, maybe some > bug fixes and who knows what else. One patch should address only one > thing. I did not review whole thing but some comments there. I also > feel like this is kinda impossible to review because so much happening. > This is over 2000 lines long patch. Do you want to review this kind of > patches yourself? I cc also fsdevel, maybe someone can help us little > bit. > Ntfs3 had data corruption. This commit is the result of reworking related functions. It's a pity, that this version wasn't in initial ntfs3 patches, because this commit basically rewrites a significant part of driver. From now on our commits will follow path "devel branch => lkml => wait for feedback => master branch" There are no plans of further refactoring of existing codebase that can lead to another huge commit. Thanks for review. > Partial review starts here: > > sfr@canb.auug.org.au > ^^ > There is this same cc. Accident? > > Subject: [PATCH] fs/ntfs3: Rework file operations > > ^^ Not very informative commit header > > Maybe better should be: > Change rename logic to add new name and then remove old one > > > Please also use at least checkpatch. It found three warnings with this > patch. > > On Tue, Aug 31, 2021 at 07:34:28PM +0300, Konstantin Komarov wrote: >> Rename now works "Add new name and remove old name". >> "Remove old name and add new name" may result in bad inode >> if we can't add new name and then can't restore (add) old name. > > Also little hard to read commit message. Let me help a little bit. > Maybe something like (not checked your patch yet): > > Current rename logic is that we first remove old name and then add new > name. This can result bad inode if there is situation that when we add > new name and it fails we have no option to return old one. > > Change behavier so that we first try to add new name and only after that > we delete old one. > >> >> Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com> >> --- >> fs/ntfs3/attrib.c | 17 +-- >> fs/ntfs3/attrlist.c | 26 ++-- >> fs/ntfs3/frecord.c | 342 ++++++++++++++++++++++++++++++++++++-------- >> fs/ntfs3/fsntfs.c | 87 +++++------ >> fs/ntfs3/index.c | 45 +++--- >> fs/ntfs3/inode.c | 275 ++++++++++++----------------------- >> fs/ntfs3/namei.c | 236 ++++++++---------------------- >> fs/ntfs3/ntfs_fs.h | 31 +++- >> fs/ntfs3/record.c | 8 +- >> fs/ntfs3/xattr.c | 25 ++-- >> 10 files changed, 563 insertions(+), 529 deletions(-) >> >> diff --git a/fs/ntfs3/attrib.c b/fs/ntfs3/attrib.c >> index 4b285f704e62..ffc323bacc9f 100644 >> --- a/fs/ntfs3/attrib.c >> +++ b/fs/ntfs3/attrib.c >> @@ -218,9 +218,11 @@ int attr_allocate_clusters(struct ntfs_sb_info *sbi, struct runs_tree *run, >> } >> >> out: >> - /* Undo. */ >> - run_deallocate_ex(sbi, run, vcn0, vcn - vcn0, NULL, false); >> - run_truncate(run, vcn0); >> + /* Undo 'ntfs_look_for_free_space' */ >> + if (vcn - vcn0) { >> + run_deallocate_ex(sbi, run, vcn0, vcn - vcn0, NULL, false); >> + run_truncate(run, vcn0); >> + } > > Can this happend for both goto out paths? If not then other should > return err straight away. > >> >> return err; >> } >> @@ -701,7 +703,7 @@ int attr_set_size(struct ntfs_inode *ni, enum ATTR_TYPE type, >> * (list entry for std attribute always first). >> * So it is safe to step back. >> */ >> - mi_remove_attr(mi, attr); >> + mi_remove_attr(NULL, mi, attr); >> >> if (!al_remove_le(ni, le)) { >> err = -EINVAL; >> @@ -1004,7 +1006,7 @@ int attr_data_get_block(struct ntfs_inode *ni, CLST vcn, CLST clen, CLST *lcn, >> end = next_svcn; >> while (end > evcn) { >> /* Remove segment [svcn : evcn). */ >> - mi_remove_attr(mi, attr); >> + mi_remove_attr(NULL, mi, attr); >> >> if (!al_remove_le(ni, le)) { >> err = -EINVAL; >> @@ -1600,7 +1602,7 @@ int attr_allocate_frame(struct ntfs_inode *ni, CLST frame, size_t compr_size, >> end = next_svcn; >> while (end > evcn) { >> /* Remove segment [svcn : evcn). */ >> - mi_remove_attr(mi, attr); >> + mi_remove_attr(NULL, mi, attr); >> >> if (!al_remove_le(ni, le)) { >> err = -EINVAL; >> @@ -1836,13 +1838,12 @@ int attr_collapse_range(struct ntfs_inode *ni, u64 vbo, u64 bytes) >> u16 le_sz; >> u16 roff = le16_to_cpu(attr->nres.run_off); >> >> - /* run==1 means unpack and deallocate. */ > > What's whit this one? Old comment maybe but should not be addressed with > this patch. > >> run_unpack_ex(RUN_DEALLOCATE, sbi, ni->mi.rno, svcn, >> evcn1 - 1, svcn, Add2Ptr(attr, roff), >> le32_to_cpu(attr->size) - roff); >> >> /* Delete this attribute segment. */ >> - mi_remove_attr(mi, attr); >> + mi_remove_attr(NULL, mi, attr); >> if (!le) >> break; >> >> diff --git a/fs/ntfs3/attrlist.c b/fs/ntfs3/attrlist.c >> index 32ca990af64b..fa32399eb517 100644 >> --- a/fs/ntfs3/attrlist.c >> +++ b/fs/ntfs3/attrlist.c >> @@ -279,7 +279,7 @@ int al_add_le(struct ntfs_inode *ni, enum ATTR_TYPE type, const __le16 *name, >> struct ATTR_LIST_ENTRY *le; >> size_t off; >> u16 sz; >> - size_t asize, new_asize; >> + size_t asize, new_asize, old_size; >> u64 new_size; >> typeof(ni->attr_list) *al = &ni->attr_list; >> >> @@ -287,8 +287,9 @@ int al_add_le(struct ntfs_inode *ni, enum ATTR_TYPE type, const __le16 *name, >> * Compute the size of the new 'le' >> */ >> sz = le_size(name_len); >> - new_size = al->size + sz; >> - asize = al_aligned(al->size); >> + old_size = al->size; >> + asize = al_aligned(old_size); >> new_asize = al_aligned(new_size); >> >> /* Scan forward to the point at which the new 'le' should be inserted. */ >> @@ -302,13 +303,14 @@ int al_add_le(struct ntfs_inode *ni, enum ATTR_TYPE type, const __le16 *name, >> return -ENOMEM; >> >> memcpy(ptr, al->le, off); >> - memcpy(Add2Ptr(ptr, off + sz), le, al->size - off); >> + memcpy(Add2Ptr(ptr, off + sz), le, old_size - off); >> le = Add2Ptr(ptr, off); >> kfree(al->le); >> al->le = ptr; >> } else { >> - memmove(Add2Ptr(le, sz), le, al->size - off); >> + memmove(Add2Ptr(le, sz), le, old_size - off); >> } >> + *new_le = le; >> >> al->size = new_size; >> >> @@ -321,23 +323,25 @@ int al_add_le(struct ntfs_inode *ni, enum ATTR_TYPE type, const __le16 *name, >> le->id = id; >> memcpy(le->name, name, sizeof(short) * name_len); >> >> - al->dirty = true; >> - >> err = attr_set_size(ni, ATTR_LIST, NULL, 0, &al->run, new_size, >> &new_size, true, &attr); >> - if (err) >> + if (err) { >> + /* Undo memmove above. */ > > There is path also for memcpy. What if we go there. Is this path then > impossible? Just checking. > >> + memmove(le, Add2Ptr(le, sz), old_size - off); >> + al->size = old_size; >> return err; >> + } >> + >> + al->dirty = true; >> >> if (attr && attr->non_res) { >> err = ntfs_sb_write_run(ni->mi.sbi, &al->run, 0, al->le, >> al->size); >> if (err) >> return err; >> + al->dirty = false; >> } >> >> - al->dirty = false; >> - *new_le = le; >> - >> return 0; >> } >> >> diff --git a/fs/ntfs3/frecord.c b/fs/ntfs3/frecord.c >> index 9d374827750b..3f48b612ec96 100644 >> --- a/fs/ntfs3/frecord.c >> +++ b/fs/ntfs3/frecord.c >> @@ -163,7 +163,7 @@ int ni_load_mi_ex(struct ntfs_inode *ni, CLST rno, struct mft_inode **mi) >> /* >> * ni_load_mi - Load mft_inode corresponded list_entry. >> */ >> -int ni_load_mi(struct ntfs_inode *ni, struct ATTR_LIST_ENTRY *le, >> +int ni_load_mi(struct ntfs_inode *ni, const struct ATTR_LIST_ENTRY *le, >> struct mft_inode **mi) > > Feels like unnecesary change for this patch. > >> { >> CLST rno; >> @@ -402,7 +402,7 @@ int ni_remove_attr(struct ntfs_inode *ni, enum ATTR_TYPE type, >> if (!attr) >> return -ENOENT; >> >> - mi_remove_attr(&ni->mi, attr); >> + mi_remove_attr(ni, &ni->mi, attr); >> return 0; >> } >> >> @@ -441,7 +441,7 @@ int ni_remove_attr(struct ntfs_inode *ni, enum ATTR_TYPE type, >> if (!attr) >> return -ENOENT; >> >> - mi_remove_attr(mi, attr); >> + mi_remove_attr(ni, mi, attr); >> >> if (PtrOffset(ni->attr_list.le, le) >= ni->attr_list.size) >> return 0; >> @@ -454,12 +454,11 @@ int ni_remove_attr(struct ntfs_inode *ni, enum ATTR_TYPE type, >> * >> * Return: Not full constructed attribute or NULL if not possible to create. >> */ >> -static struct ATTRIB *ni_ins_new_attr(struct ntfs_inode *ni, >> - struct mft_inode *mi, >> - struct ATTR_LIST_ENTRY *le, >> - enum ATTR_TYPE type, const __le16 *name, >> - u8 name_len, u32 asize, u16 name_off, >> - CLST svcn) >> +static struct ATTRIB * >> +ni_ins_new_attr(struct ntfs_inode *ni, struct mft_inode *mi, >> + struct ATTR_LIST_ENTRY *le, enum ATTR_TYPE type, >> + const __le16 *name, u8 name_len, u32 asize, u16 name_off, >> + CLST svcn, struct ATTR_LIST_ENTRY **ins_le) > > Reviewing would be lot lot easier if you make one patch which just adds > this new ins_le to every function needed. Just call it with NULL. Then > there would be much less noise to watch. > >> { >> int err; >> struct ATTRIB *attr; >> @@ -507,6 +506,8 @@ static struct ATTRIB *ni_ins_new_attr(struct ntfs_inode *ni, >> le->ref = ref; >> >> out: >> + if (ins_le) >> + *ins_le = le; >> return attr; >> } >> >> @@ -599,7 +600,7 @@ static int ni_repack(struct ntfs_inode *ni) >> if (next_svcn >= evcn + 1) { >> /* We can remove this attribute segment. */ >> al_remove_le(ni, le); >> - mi_remove_attr(mi, attr); >> + mi_remove_attr(NULL, mi, attr); >> le = le_p; >> continue; >> } >> @@ -695,8 +696,8 @@ static int ni_try_remove_attr_list(struct ntfs_inode *ni) >> free -= asize; >> } >> >> - /* Is seems that attribute list can be removed from primary record. */ >> - mi_remove_attr(&ni->mi, attr_list); >> + /* It seems that attribute list can be removed from primary record. */ >> + mi_remove_attr(NULL, &ni->mi, attr_list); >> >> /* >> * Repeat the cycle above and move all attributes to primary record. >> @@ -724,7 +725,7 @@ static int ni_try_remove_attr_list(struct ntfs_inode *ni) >> attr_ins->id = id; >> >> /* Remove from original record. */ >> - mi_remove_attr(mi, attr); >> + mi_remove_attr(NULL, mi, attr); >> } >> >> run_deallocate(sbi, &ni->attr_list.run, true); >> @@ -842,7 +843,8 @@ int ni_create_attr_list(struct ntfs_inode *ni) >> memcpy(attr, b, asize); >> attr->id = le_b[nb]->id; >> >> - WARN_ON(!mi_remove_attr(&ni->mi, b)); >> + /* Remove from primary record. */ >> + WARN_ON(!mi_remove_attr(NULL, &ni->mi, b)); >> >> if (to_free <= asize) >> break; >> @@ -883,7 +885,8 @@ int ni_create_attr_list(struct ntfs_inode *ni) >> static int ni_ins_attr_ext(struct ntfs_inode *ni, struct ATTR_LIST_ENTRY *le, >> enum ATTR_TYPE type, const __le16 *name, u8 name_len, >> u32 asize, CLST svcn, u16 name_off, bool force_ext, >> - struct ATTRIB **ins_attr, struct mft_inode **ins_mi) >> + struct ATTRIB **ins_attr, struct mft_inode **ins_mi, >> + struct ATTR_LIST_ENTRY **ins_le) >> { >> struct ATTRIB *attr; >> struct mft_inode *mi; >> @@ -956,12 +959,14 @@ static int ni_ins_attr_ext(struct ntfs_inode *ni, struct ATTR_LIST_ENTRY *le, >> >> /* Try to insert attribute into this subrecord. */ >> attr = ni_ins_new_attr(ni, mi, le, type, name, name_len, asize, >> - name_off, svcn); >> + name_off, svcn, ins_le); >> if (!attr) >> continue; >> >> if (ins_attr) >> *ins_attr = attr; >> + if (ins_mi) >> + *ins_mi = mi; >> return 0; >> } >> >> @@ -977,7 +982,7 @@ static int ni_ins_attr_ext(struct ntfs_inode *ni, struct ATTR_LIST_ENTRY *le, >> } >> >> attr = ni_ins_new_attr(ni, mi, le, type, name, name_len, asize, >> - name_off, svcn); >> + name_off, svcn, ins_le); >> if (!attr) >> goto out2; >> >> @@ -1016,7 +1021,8 @@ static int ni_ins_attr_ext(struct ntfs_inode *ni, struct ATTR_LIST_ENTRY *le, >> static int ni_insert_attr(struct ntfs_inode *ni, enum ATTR_TYPE type, >> const __le16 *name, u8 name_len, u32 asize, >> u16 name_off, CLST svcn, struct ATTRIB **ins_attr, >> - struct mft_inode **ins_mi) >> + struct mft_inode **ins_mi, >> + struct ATTR_LIST_ENTRY **ins_le) >> { >> struct ntfs_sb_info *sbi = ni->mi.sbi; >> int err; >> @@ -1045,7 +1051,7 @@ static int ni_insert_attr(struct ntfs_inode *ni, enum ATTR_TYPE type, >> >> if (asize <= free) { >> attr = ni_ins_new_attr(ni, &ni->mi, NULL, type, name, name_len, >> - asize, name_off, svcn); >> + asize, name_off, svcn, ins_le); >> if (attr) { >> if (ins_attr) >> *ins_attr = attr; >> @@ -1059,7 +1065,8 @@ static int ni_insert_attr(struct ntfs_inode *ni, enum ATTR_TYPE type, >> if (!is_mft || type != ATTR_DATA || svcn) { >> /* This ATTRIB will be external. */ >> err = ni_ins_attr_ext(ni, NULL, type, name, name_len, asize, >> - svcn, name_off, false, ins_attr, ins_mi); >> + svcn, name_off, false, ins_attr, ins_mi, >> + ins_le); >> goto out; >> } >> >> @@ -1117,7 +1124,7 @@ static int ni_insert_attr(struct ntfs_inode *ni, enum ATTR_TYPE type, >> t16 = le16_to_cpu(attr->name_off); >> err = ni_ins_attr_ext(ni, le, attr->type, Add2Ptr(attr, t16), >> attr->name_len, t32, attr_svcn(attr), t16, >> - false, &eattr, NULL); >> + false, &eattr, NULL, NULL); >> if (err) >> return err; >> >> @@ -1125,8 +1132,8 @@ static int ni_insert_attr(struct ntfs_inode *ni, enum ATTR_TYPE type, >> memcpy(eattr, attr, t32); >> eattr->id = id; >> >> - /* Remove attrib from primary record. */ >> - mi_remove_attr(&ni->mi, attr); >> + /* Remove from primary record. */ >> + mi_remove_attr(NULL, &ni->mi, attr); >> >> /* attr now points to next attribute. */ >> if (attr->type == ATTR_END) >> @@ -1136,7 +1143,7 @@ static int ni_insert_attr(struct ntfs_inode *ni, enum ATTR_TYPE type, >> ; >> >> attr = ni_ins_new_attr(ni, &ni->mi, NULL, type, name, name_len, asize, >> - name_off, svcn); >> + name_off, svcn, ins_le); >> if (!attr) { >> err = -EINVAL; >> goto out; >> @@ -1251,7 +1258,7 @@ static int ni_expand_mft_list(struct ntfs_inode *ni) >> */ >> attr = ni_ins_new_attr(ni, mi_min, NULL, ATTR_DATA, NULL, 0, >> SIZEOF_NONRESIDENT + run_size, >> - SIZEOF_NONRESIDENT, svcn); >> + SIZEOF_NONRESIDENT, svcn, NULL); >> if (!attr) { >> err = -EINVAL; >> goto out; >> @@ -1315,14 +1322,15 @@ int ni_expand_list(struct ntfs_inode *ni) >> err = ni_ins_attr_ext(ni, le, attr->type, attr_name(attr), >> attr->name_len, asize, attr_svcn(attr), >> le16_to_cpu(attr->name_off), true, >> - &ins_attr, NULL); >> + &ins_attr, NULL, NULL); >> >> if (err) >> goto out; >> >> memcpy(ins_attr, attr, asize); >> ins_attr->id = le->id; >> - mi_remove_attr(&ni->mi, attr); >> + /* Remove from primary record. */ >> + mi_remove_attr(NULL, &ni->mi, attr); >> >> done += asize; >> goto out; >> @@ -1382,7 +1390,7 @@ int ni_insert_nonresident(struct ntfs_inode *ni, enum ATTR_TYPE type, >> } >> >> err = ni_insert_attr(ni, type, name, name_len, asize, name_off, svcn, >> - &attr, mi); >> + &attr, mi, NULL); >> >> if (err) >> goto out; >> @@ -1422,7 +1430,8 @@ int ni_insert_nonresident(struct ntfs_inode *ni, enum ATTR_TYPE type, >> */ >> int ni_insert_resident(struct ntfs_inode *ni, u32 data_size, >> enum ATTR_TYPE type, const __le16 *name, u8 name_len, >> - struct ATTRIB **new_attr, struct mft_inode **mi) >> + struct ATTRIB **new_attr, struct mft_inode **mi, >> + struct ATTR_LIST_ENTRY **le) >> { >> int err; >> u32 name_size = ALIGN(name_len * sizeof(short), 8); >> @@ -1430,7 +1439,7 @@ int ni_insert_resident(struct ntfs_inode *ni, u32 data_size, >> struct ATTRIB *attr; >> >> err = ni_insert_attr(ni, type, name, name_len, asize, SIZEOF_RESIDENT, >> - 0, &attr, mi); >> + 0, &attr, mi, le); >> if (err) >> return err; >> >> @@ -1439,8 +1448,13 @@ int ni_insert_resident(struct ntfs_inode *ni, u32 data_size, >> >> attr->res.data_size = cpu_to_le32(data_size); >> attr->res.data_off = cpu_to_le16(SIZEOF_RESIDENT + name_size); >> - if (type == ATTR_NAME) >> + if (type == ATTR_NAME) { >> attr->res.flags = RESIDENT_FLAG_INDEXED; >> + >> + /* is_attr_indexed(attr)) == true */ >> + le16_add_cpu(&ni->mi.mrec->hard_links, +1); >> + ni->mi.dirty = true; >> + } >> attr->res.res = 0; >> >> if (new_attr) >> @@ -1452,22 +1466,13 @@ int ni_insert_resident(struct ntfs_inode *ni, u32 data_size, >> /* >> * ni_remove_attr_le - Remove attribute from record. >> */ >> -int ni_remove_attr_le(struct ntfs_inode *ni, struct ATTRIB *attr, >> - struct ATTR_LIST_ENTRY *le) >> +void ni_remove_attr_le(struct ntfs_inode *ni, struct ATTRIB *attr, >> + struct mft_inode *mi, struct ATTR_LIST_ENTRY *le) >> { >> - int err; >> - struct mft_inode *mi; >> - >> - err = ni_load_mi(ni, le, &mi); >> - if (err) >> - return err; >> - >> - mi_remove_attr(mi, attr); >> + mi_remove_attr(ni, mi, attr); >> >> if (le) >> al_remove_le(ni, le); >> - >> - return 0; >> } >> >> /* >> @@ -1549,10 +1554,12 @@ int ni_delete_all(struct ntfs_inode *ni) >> >> /* ni_fname_name >> * >> - *Return: File name attribute by its value. */ >> + * Return: File name attribute by its value. >> + */ >> struct ATTR_FILE_NAME *ni_fname_name(struct ntfs_inode *ni, >> const struct cpu_str *uni, >> const struct MFT_REF *home_dir, >> + struct mft_inode **mi, >> struct ATTR_LIST_ENTRY **le) >> { >> struct ATTRIB *attr = NULL; >> @@ -1562,7 +1569,7 @@ struct ATTR_FILE_NAME *ni_fname_name(struct ntfs_inode *ni, >> >> /* Enumerate all names. */ >> next: >> - attr = ni_find_attr(ni, attr, le, ATTR_NAME, NULL, 0, NULL, NULL); >> + attr = ni_find_attr(ni, attr, le, ATTR_NAME, NULL, 0, NULL, mi); >> if (!attr) >> return NULL; >> >> @@ -1592,6 +1599,7 @@ struct ATTR_FILE_NAME *ni_fname_name(struct ntfs_inode *ni, >> * Return: File name attribute with given type. >> */ >> struct ATTR_FILE_NAME *ni_fname_type(struct ntfs_inode *ni, u8 name_type, >> + struct mft_inode **mi, >> struct ATTR_LIST_ENTRY **le) >> { >> struct ATTRIB *attr = NULL; >> @@ -1599,10 +1607,12 @@ struct ATTR_FILE_NAME *ni_fname_type(struct ntfs_inode *ni, u8 name_type, >> >> *le = NULL; >> >> + if (FILE_NAME_POSIX == name_type) >> + return NULL; >> + >> /* Enumerate all names. */ >> for (;;) { >> - attr = ni_find_attr(ni, attr, le, ATTR_NAME, NULL, 0, NULL, >> - NULL); >> + attr = ni_find_attr(ni, attr, le, ATTR_NAME, NULL, 0, NULL, mi); >> if (!attr) >> return NULL; >> >> @@ -2788,6 +2798,222 @@ int ni_write_frame(struct ntfs_inode *ni, struct page **pages, >> return err; >> } >> >> +/* >> + * ni_remove_name - Removes name 'de' from MFT and from directory. >> + * 'de2' and 'undo_step' are used to restore MFT/dir, if error occurs. >> + */ >> +int ni_remove_name(struct ntfs_inode *dir_ni, struct ntfs_inode *ni, >> + struct NTFS_DE *de, struct NTFS_DE **de2, int *undo_step) >> +{ >> + int err; >> + struct ntfs_sb_info *sbi = ni->mi.sbi; >> + struct ATTR_FILE_NAME *de_name = (struct ATTR_FILE_NAME *)(de + 1); >> + struct ATTR_FILE_NAME *fname; >> + struct ATTR_LIST_ENTRY *le; >> + struct mft_inode *mi; >> + u16 de_key_size = le16_to_cpu(de->key_size); >> + u8 name_type; >> + >> + *undo_step = 0; > > It looks to me that this function can make undo by itself. There is no > need for ni_remove_name_undo. This looks very strange way to do this. > > One other way if really needed is also to use return value, but this > pointer undo_step feels just wrong. > >> + >> + /* Find name in record. */ >> + mi_get_ref(&dir_ni->mi, &de_name->home); >> + >> + fname = ni_fname_name(ni, (struct cpu_str *)&de_name->name_len, >> + &de_name->home, &mi, &le); >> + if (!fname) >> + return -ENOENT; >> + >> + memcpy(&de_name->dup, &fname->dup, sizeof(struct NTFS_DUP_INFO)); >> + name_type = paired_name(fname->type); >> + >> + /* Mark ntfs as dirty. It will be cleared at umount. */ >> + ntfs_set_state(sbi, NTFS_DIRTY_DIRTY); >> + >> + /* Step 1: Remove name from directory. */ >> + err = indx_delete_entry(&dir_ni->dir, dir_ni, fname, de_key_size, sbi); >> + if (err) >> + return err; >> + >> + /* Step 2: Remove name from MFT. */ >> + ni_remove_attr_le(ni, attr_from_name(fname), mi, le); >> + >> + *undo_step = 2; >> + >> + /* Get paired name. */ >> + fname = ni_fname_type(ni, name_type, &mi, &le); > > It does not feel right that ni_fname_type does not return anything if > name_type is FILE_NAME_POSIX. It can do it right? Why won't we check > before ni_fname_type call if name_type == FILE_NAME_POSIX and just exit > with return 0 if it is. > >> + if (fname) { >> + u16 de2_key_size = fname_full_size(fname); >> + >> + *de2 = Add2Ptr(de, 1024); >> + (*de2)->key_size = cpu_to_le16(de2_key_size); >> + >> + memcpy(*de2 + 1, fname, de2_key_size); >> + >> + /* Step 3: Remove paired name from directory. */ >> + err = indx_delete_entry(&dir_ni->dir, dir_ni, fname, >> + de2_key_size, sbi); >> + if (err) >> + return err; >> + >> + /* Step 4: Remove paired name from MFT. */ >> + ni_remove_attr_le(ni, attr_from_name(fname), mi, le); >> + >> + *undo_step = 4; >> + } >> + return 0; >> +} >> + >> +/* >> + * ni_remove_name_undo - Paired function for ni_remove_name. >> + * >> + * Return: True if ok >> + */ >> +bool ni_remove_name_undo(struct ntfs_inode *dir_ni, struct ntfs_inode *ni, >> + struct NTFS_DE *de, struct NTFS_DE *de2, int undo_step) >> +{ >> + struct ntfs_sb_info *sbi = ni->mi.sbi; >> + struct ATTRIB *attr; >> + u16 de_key_size = de2 ? le16_to_cpu(de2->key_size) : 0; >> + >> + switch (undo_step) { >> + case 4: >> + if (ni_insert_resident(ni, de_key_size, ATTR_NAME, NULL, 0, >> + &attr, NULL, NULL)) { >> + return false; >> + } >> + memcpy(Add2Ptr(attr, SIZEOF_RESIDENT), de2 + 1, de_key_size); >> + >> + mi_get_ref(&ni->mi, &de2->ref); >> + de2->size = cpu_to_le16(ALIGN(de_key_size, 8) + >> + sizeof(struct NTFS_DE)); >> + de2->flags = 0; >> + de2->res = 0; >> + >> + if (indx_insert_entry(&dir_ni->dir, dir_ni, de2, sbi, NULL, >> + 1)) { >> + return false; >> + } >> + fallthrough; >> + >> + case 2: >> + de_key_size = le16_to_cpu(de->key_size); >> + >> + if (ni_insert_resident(ni, de_key_size, ATTR_NAME, NULL, 0, >> + &attr, NULL, NULL)) { >> + return false; >> + } >> + >> + memcpy(Add2Ptr(attr, SIZEOF_RESIDENT), de + 1, de_key_size); >> + mi_get_ref(&ni->mi, &de->ref); >> + >> + if (indx_insert_entry(&dir_ni->dir, dir_ni, de, sbi, NULL, 1)) { >> + return false; >> + } >> + } >> + >> + return true; >> +} >> + >> +/* >> + * ni_add_name - Add new name in MFT and in directory. > > *to would be better? Also in comments. > >> + */ >> +int ni_add_name(struct ntfs_inode *dir_ni, struct ntfs_inode *ni, >> + struct NTFS_DE *de) >> +{ >> + int err; >> + struct ATTRIB *attr; >> + struct ATTR_LIST_ENTRY *le; >> + struct mft_inode *mi; >> + struct ATTR_FILE_NAME *de_name = (struct ATTR_FILE_NAME *)(de + 1); >> + u16 de_key_size = le16_to_cpu(de->key_size); >> + >> + mi_get_ref(&ni->mi, &de->ref); >> + mi_get_ref(&dir_ni->mi, &de_name->home); >> + >> + /* Insert new name in MFT. */ >> + err = ni_insert_resident(ni, de_key_size, ATTR_NAME, NULL, 0, &attr, >> + &mi, &le); >> + if (err) >> + return err; >> + >> + memcpy(Add2Ptr(attr, SIZEOF_RESIDENT), de_name, de_key_size); >> + >> + /* Insert new name in directory. */ >> + err = indx_insert_entry(&dir_ni->dir, dir_ni, de, ni->mi.sbi, NULL, 0); >> + if (err) >> + ni_remove_attr_le(ni, attr, mi, le); >> + >> + return err; >> +} >> + >> +/* >> + * ni_rename - Remove one name and insert new name. >> + */ >> +int ni_rename(struct ntfs_inode *dir_ni, struct ntfs_inode *new_dir_ni, >> + struct ntfs_inode *ni, struct NTFS_DE *de, struct NTFS_DE *new_de, >> + bool *is_bad) >> +{ >> + int err; >> + struct NTFS_DE *de2 = NULL; >> + int undo = 0; >> + >> + /* >> + * There are two possible ways to rename: >> + * 1) Add new name and remove old name. >> + * 2) Remove old name and add new name. >> + * >> + * In most cases (not all!) adding new name in MFT and in directory can >> + * allocate additional cluster(s). >> + * Second way may result to bad inode if we can't add new name >> + * and then can't restore (add) old name. >> + */ >> + >> + /* >> + * Way 1 - Add new + remove old. >> + */ > > Way too big comment for thing this small. > >> + err = ni_add_name(new_dir_ni, ni, new_de); > > if (err) > return err; > >> + if (!err) { >> + err = ni_remove_name(dir_ni, ni, de, &de2, &undo); >> + if (err && ni_remove_name(new_dir_ni, ni, new_de, &de2, &undo)) >> + *is_bad = true; > > Maybe we can return errno or 1 here. is_bad is again imo little > unnecessary here. And now that I think this maybe this should be in > caller site in ntfs_rename. > >> + } >> + >> + /* >> + * Way 2 - Remove old + add new. >> + */ >> + /* >> + * err = ni_remove_name(dir_ni, ni, de, &de2, &undo); >> + * if (!err) { >> + * err = ni_add_name(new_dir_ni, ni, new_de); >> + * if (err && !ni_remove_name_undo(dir_ni, ni, de, de2, undo)) >> + * *is_bad = true; >> + * } >> + */ >> + >> + return err; >> +} >> + >> +/* >> + * ni_is_dirty - Return: True if 'ni' requires ni_write_inode. >> + */ >> +bool ni_is_dirty(struct inode *inode) >> +{ >> + struct ntfs_inode *ni = ntfs_i(inode); >> + struct rb_node *node; >> + >> + if (ni->mi.dirty || ni->attr_list.dirty || >> + (ni->ni_flags & NI_FLAG_UPDATE_PARENT)) > > No need to save space here. Just another if statment for third one. > >> + return true; >> + >> + for (node = rb_first(&ni->mi_tree); node; node = rb_next(node)) { >> + if (rb_entry(node, struct mft_inode, node)->dirty) >> + return true; >> + } >> + >> + return false; >> +} >> + >> /* >> * ni_update_parent >> * >> @@ -2802,8 +3028,6 @@ static bool ni_update_parent(struct ntfs_inode *ni, struct NTFS_DUP_INFO *dup, >> struct ntfs_sb_info *sbi = ni->mi.sbi; >> struct super_block *sb = sbi->sb; >> bool re_dirty = false; >> - bool active = sb->s_flags & SB_ACTIVE; >> - bool upd_parent = ni->ni_flags & NI_FLAG_UPDATE_PARENT; >> >> if (ni->mi.mrec->flags & RECORD_FLAG_DIR) { >> dup->fa |= FILE_ATTRIBUTE_DIRECTORY; >> @@ -2867,19 +3091,9 @@ static bool ni_update_parent(struct ntfs_inode *ni, struct NTFS_DUP_INFO *dup, >> struct ATTR_FILE_NAME *fname; >> >> fname = resident_data_ex(attr, SIZEOF_ATTRIBUTE_FILENAME); >> - if (!fname) >> + if (!fname || !memcmp(&fname->dup, dup, sizeof(fname->dup))) >> continue; >> >> - if (memcmp(&fname->dup, dup, sizeof(fname->dup))) { >> - memcpy(&fname->dup, dup, sizeof(fname->dup)); >> - mi->dirty = true; >> - } else if (!upd_parent) { >> - continue; >> - } >> - >> - if (!active) >> - continue; /* Avoid __wait_on_freeing_inode(inode); */ >> - >> /* ntfs_iget5 may sleep. */ >> dir = ntfs_iget5(sb, &fname->home, NULL); >> if (IS_ERR(dir)) { >> @@ -2898,6 +3112,8 @@ static bool ni_update_parent(struct ntfs_inode *ni, struct NTFS_DUP_INFO *dup, >> } else { >> indx_update_dup(dir_ni, sbi, fname, dup, sync); >> ni_unlock(dir_ni); >> + memcpy(&fname->dup, dup, sizeof(fname->dup)); >> + mi->dirty = true; >> } >> } >> iput(dir); >> @@ -2969,7 +3185,9 @@ int ni_write_inode(struct inode *inode, int sync, const char *hint) >> ni->mi.dirty = true; >> >> if (!ntfs_is_meta_file(sbi, inode->i_ino) && >> - (modified || (ni->ni_flags & NI_FLAG_UPDATE_PARENT))) { >> + (modified || (ni->ni_flags & NI_FLAG_UPDATE_PARENT)) >> + /* Avoid __wait_on_freeing_inode(inode). */ >> + && (sb->s_flags & SB_ACTIVE)) { >> dup.cr_time = std->cr_time; >> /* Not critical if this function fail. */ >> re_dirty = ni_update_parent(ni, &dup, sync); >> @@ -3033,7 +3251,7 @@ int ni_write_inode(struct inode *inode, int sync, const char *hint) >> return err; >> } >> >> - if (re_dirty && (sb->s_flags & SB_ACTIVE)) >> + if (re_dirty) >> mark_inode_dirty_sync(inode); >> >> return 0; >> diff --git a/fs/ntfs3/fsntfs.c b/fs/ntfs3/fsntfs.c >> index 0edb95ed9717..669249439217 100644 >> --- a/fs/ntfs3/fsntfs.c >> +++ b/fs/ntfs3/fsntfs.c >> @@ -358,29 +358,25 @@ int ntfs_look_for_free_space(struct ntfs_sb_info *sbi, CLST lcn, CLST len, >> enum ALLOCATE_OPT opt) >> { >> int err; >> + CLST alen = 0; >> struct super_block *sb = sbi->sb; >> - size_t a_lcn, zlen, zeroes, zlcn, zlen2, ztrim, new_zlen; >> + size_t alcn, zlen, zeroes, zlcn, zlen2, ztrim, new_zlen; >> struct wnd_bitmap *wnd = &sbi->used.bitmap; >> >> down_write_nested(&wnd->rw_lock, BITMAP_MUTEX_CLUSTERS); >> if (opt & ALLOCATE_MFT) { >> - CLST alen; >> - >> zlen = wnd_zone_len(wnd); >> >> if (!zlen) { >> err = ntfs_refresh_zone(sbi); >> if (err) >> goto out; > > Before we return this error code. Now we return -ENOSPC if this error. > >> - >> zlen = wnd_zone_len(wnd); >> + } >> >> - if (!zlen) { >> - ntfs_err(sbi->sb, >> - "no free space to extend mft"); >> - err = -ENOSPC; >> - goto out; >> - } >> + if (!zlen) { >> + ntfs_err(sbi->sb, "no free space to extend mft"); >> + goto out; > > This is just refactoring and should do in different patch. > >> } >> >> lcn = wnd_zone_bit(wnd); >> @@ -389,14 +385,13 @@ int ntfs_look_for_free_space(struct ntfs_sb_info *sbi, CLST lcn, CLST len, >> wnd_zone_set(wnd, lcn + alen, zlen - alen); >> >> err = wnd_set_used(wnd, lcn, alen); >> - if (err) >> - goto out; >> - >> - *new_lcn = lcn; >> - *new_len = alen; >> - goto ok; >> + if (err) { >> + up_write(&wnd->rw_lock); >> + return err; >> + } >> + alcn = lcn; >> + goto out; >> } >> - >> /* >> * 'Cause cluster 0 is always used this value means that we should use >> * cached value of 'next_free_lcn' to improve performance. >> @@ -407,22 +402,17 @@ int ntfs_look_for_free_space(struct ntfs_sb_info *sbi, CLST lcn, CLST len, >> if (lcn >= wnd->nbits) >> lcn = 0; >> >> - *new_len = wnd_find(wnd, len, lcn, BITMAP_FIND_MARK_AS_USED, &a_lcn); >> - if (*new_len) { >> - *new_lcn = a_lcn; >> - goto ok; >> - } >> + alen = wnd_find(wnd, len, lcn, BITMAP_FIND_MARK_AS_USED, &alcn); >> + if (alen) >> + goto out; >> >> /* Try to use clusters from MftZone. */ >> zlen = wnd_zone_len(wnd); >> zeroes = wnd_zeroes(wnd); >> >> - /* Check too big request. */ >> - if (len > zeroes + zlen) >> - goto no_space; >> - >> - if (zlen <= NTFS_MIN_MFT_ZONE) >> - goto no_space; >> + /* Check too big request */ >> + if (len > zeroes + zlen || zlen <= NTFS_MIN_MFT_ZONE) >> + goto out; >> >> /* How many clusters to cat from zone. */ >> zlcn = wnd_zone_bit(wnd); >> @@ -439,31 +429,24 @@ int ntfs_look_for_free_space(struct ntfs_sb_info *sbi, CLST lcn, CLST len, >> wnd_zone_set(wnd, zlcn, new_zlen); >> >> /* Allocate continues clusters. */ >> - *new_len = >> - wnd_find(wnd, len, 0, >> - BITMAP_FIND_MARK_AS_USED | BITMAP_FIND_FULL, &a_lcn); >> - if (*new_len) { >> - *new_lcn = a_lcn; >> - goto ok; >> - } >> - >> -no_space: >> - up_write(&wnd->rw_lock); >> - >> - return -ENOSPC; >> - >> -ok: >> - err = 0; >> + alen = wnd_find(wnd, len, 0, >> + BITMAP_FIND_MARK_AS_USED | BITMAP_FIND_FULL, &alcn); >> >> - ntfs_unmap_meta(sb, *new_lcn, *new_len); >> +out: >> + if (alen) { > > Just use more goto if needed. This is hard to follow when we come here > and when not. > >> + err = 0; >> + *new_len = alen; >> + *new_lcn = alcn; >> >> - if (opt & ALLOCATE_MFT) >> - goto out; >> + ntfs_unmap_meta(sb, alcn, alen); >> >> - /* Set hint for next requests. */ >> - sbi->used.next_free_lcn = *new_lcn + *new_len; >> + /* Set hint for next requests. */ >> + if (!(opt & ALLOCATE_MFT)) >> + sbi->used.next_free_lcn = alcn + alen; >> + } else { >> + err = -ENOSPC; >> + } >> >> -out: > > Example here > > err_up_write: > > and you can use that for many place > >> up_write(&wnd->rw_lock); >> return err; >> } >> @@ -2226,7 +2209,7 @@ int ntfs_insert_security(struct ntfs_sb_info *sbi, >> sii_e.sec_id = d_security->key.sec_id; >> memcpy(&sii_e.sec_hdr, d_security, SIZEOF_SECURITY_HDR); >> >> - err = indx_insert_entry(indx_sii, ni, &sii_e.de, NULL, NULL); >> + err = indx_insert_entry(indx_sii, ni, &sii_e.de, NULL, NULL, 0); >> if (err) >> goto out; >> >> @@ -2247,7 +2230,7 @@ int ntfs_insert_security(struct ntfs_sb_info *sbi, >> >> fnd_clear(fnd_sdh); >> err = indx_insert_entry(indx_sdh, ni, &sdh_e.de, (void *)(size_t)1, >> - fnd_sdh); >> + fnd_sdh, 0); >> if (err) >> goto out; >> >> @@ -2385,7 +2368,7 @@ int ntfs_insert_reparse(struct ntfs_sb_info *sbi, __le32 rtag, >> >> mutex_lock_nested(&ni->ni_lock, NTFS_INODE_MUTEX_REPARSE); >> >> - err = indx_insert_entry(indx, ni, &re.de, NULL, NULL); >> + err = indx_insert_entry(indx, ni, &re.de, NULL, NULL, 0); >> >> mark_inode_dirty(&ni->vfs_inode); >> ni_unlock(ni); >> diff --git a/fs/ntfs3/index.c b/fs/ntfs3/index.c >> index 70ef59455b72..1224b8e42b3e 100644 >> --- a/fs/ntfs3/index.c >> +++ b/fs/ntfs3/index.c >> @@ -1427,7 +1427,7 @@ static int indx_create_allocate(struct ntfs_index *indx, struct ntfs_inode *ni, >> alloc->nres.valid_size = alloc->nres.data_size = cpu_to_le64(data_size); >> >> err = ni_insert_resident(ni, bitmap_size(1), ATTR_BITMAP, in->name, >> - in->name_len, &bitmap, NULL); >> + in->name_len, &bitmap, NULL, NULL); >> if (err) >> goto out2; >> >> @@ -1443,7 +1443,7 @@ static int indx_create_allocate(struct ntfs_index *indx, struct ntfs_inode *ni, >> return 0; >> >> out2: >> - mi_remove_attr(&ni->mi, alloc); >> + mi_remove_attr(NULL, &ni->mi, alloc); >> >> out1: >> run_deallocate(sbi, &run, false); >> @@ -1529,24 +1529,24 @@ static int indx_add_allocate(struct ntfs_index *indx, struct ntfs_inode *ni, >> /* >> * indx_insert_into_root - Attempt to insert an entry into the index root. >> * >> + * @undo - True if we undoing previous remove. >> * If necessary, it will twiddle the index b-tree. >> */ >> static int indx_insert_into_root(struct ntfs_index *indx, struct ntfs_inode *ni, >> const struct NTFS_DE *new_de, >> struct NTFS_DE *root_de, const void *ctx, >> - struct ntfs_fnd *fnd) >> + struct ntfs_fnd *fnd, bool undo) >> { >> int err = 0; >> struct NTFS_DE *e, *e0, *re; >> struct mft_inode *mi; >> struct ATTRIB *attr; >> - struct MFT_REC *rec; >> struct INDEX_HDR *hdr; >> struct indx_node *n; >> CLST new_vbn; >> __le64 *sub_vbn, t_vbn; >> u16 new_de_size; >> - u32 hdr_used, hdr_total, asize, used, to_move; >> + u32 hdr_used, hdr_total, asize, to_move; >> u32 root_size, new_root_size; >> struct ntfs_sb_info *sbi; >> int ds_root; >> @@ -1559,12 +1559,11 @@ static int indx_insert_into_root(struct ntfs_index *indx, struct ntfs_inode *ni, >> >> /* >> * Try easy case: >> - * hdr_insert_de will succeed if there's room the root for the new entry. >> + * hdr_insert_de will succeed if there's >> + * room the root for the new entry. > > Not relevant for this patch. > >> */ >> hdr = &root->ihdr; >> sbi = ni->mi.sbi; >> - rec = mi->mrec; >> - used = le32_to_cpu(rec->used); >> new_de_size = le16_to_cpu(new_de->size); >> hdr_used = le32_to_cpu(hdr->used); >> hdr_total = le32_to_cpu(hdr->total); >> @@ -1573,9 +1572,9 @@ static int indx_insert_into_root(struct ntfs_index *indx, struct ntfs_inode *ni, >> >> ds_root = new_de_size + hdr_used - hdr_total; >> >> - if (used + ds_root < sbi->max_bytes_per_attr) { >> - /* Make a room for new elements. */ >> - mi_resize_attr(mi, attr, ds_root); >> + /* If 'undo' is set then reduce requirements. */ >> + if ((undo || asize + ds_root < sbi->max_bytes_per_attr) && >> + mi_resize_attr(mi, attr, ds_root)) { >> hdr->total = cpu_to_le32(hdr_total + ds_root); >> e = hdr_insert_de(indx, hdr, new_de, root_de, ctx); >> WARN_ON(!e); >> @@ -1629,7 +1628,7 @@ static int indx_insert_into_root(struct ntfs_index *indx, struct ntfs_inode *ni, >> sizeof(u64); >> ds_root = new_root_size - root_size; >> >> - if (ds_root > 0 && used + ds_root > sbi->max_bytes_per_attr) { >> + if (ds_root > 0 && asize + ds_root > sbi->max_bytes_per_attr) { >> /* Make root external. */ >> err = -EOPNOTSUPP; >> goto out_free_re; >> @@ -1710,7 +1709,7 @@ static int indx_insert_into_root(struct ntfs_index *indx, struct ntfs_inode *ni, >> >> put_indx_node(n); >> fnd_clear(fnd); >> - err = indx_insert_entry(indx, ni, new_de, ctx, fnd); >> + err = indx_insert_entry(indx, ni, new_de, ctx, fnd, undo); >> goto out_free_root; >> } >> >> @@ -1854,7 +1853,7 @@ indx_insert_into_buffer(struct ntfs_index *indx, struct ntfs_inode *ni, >> */ >> if (!level) { >> /* Insert in root. */ >> - err = indx_insert_into_root(indx, ni, up_e, NULL, ctx, fnd); >> + err = indx_insert_into_root(indx, ni, up_e, NULL, ctx, fnd, 0); >> if (err) >> goto out; >> } else { >> @@ -1876,10 +1875,12 @@ indx_insert_into_buffer(struct ntfs_index *indx, struct ntfs_inode *ni, >> >> /* >> * indx_insert_entry - Insert new entry into index. >> + * >> + * @undo - True if we undoing previous remove. >> */ >> int indx_insert_entry(struct ntfs_index *indx, struct ntfs_inode *ni, >> const struct NTFS_DE *new_de, const void *ctx, >> - struct ntfs_fnd *fnd) >> + struct ntfs_fnd *fnd, bool undo) >> { >> int err; >> int diff; >> @@ -1925,7 +1926,7 @@ int indx_insert_entry(struct ntfs_index *indx, struct ntfs_inode *ni, >> * new entry into it. >> */ >> err = indx_insert_into_root(indx, ni, new_de, fnd->root_de, ctx, >> - fnd); >> + fnd, undo); >> if (err) >> goto out; >> } else { >> @@ -2302,7 +2303,7 @@ int indx_delete_entry(struct ntfs_index *indx, struct ntfs_inode *ni, >> fnd->level - 1, >> fnd) >> : indx_insert_into_root(indx, ni, re, e, >> - ctx, fnd); >> + ctx, fnd, 0); >> kfree(re); >> >> if (err) >> @@ -2507,7 +2508,7 @@ int indx_delete_entry(struct ntfs_index *indx, struct ntfs_inode *ni, >> * Re-insert the entry into the tree. >> * Find the spot the tree where we want to insert the new entry. >> */ >> - err = indx_insert_entry(indx, ni, me, ctx, fnd); >> + err = indx_insert_entry(indx, ni, me, ctx, fnd, 0); >> kfree(me); >> if (err) >> goto out; >> @@ -2595,10 +2596,8 @@ int indx_update_dup(struct ntfs_inode *ni, struct ntfs_sb_info *sbi, >> struct ntfs_index *indx = &ni->dir; >> >> fnd = fnd_get(); >> - if (!fnd) { >> - err = -ENOMEM; >> - goto out1; >> - } >> + if (!fnd) >> + return -ENOMEM; >> >> root = indx_get_root(indx, ni, NULL, &mi); >> if (!root) { >> @@ -2645,7 +2644,5 @@ int indx_update_dup(struct ntfs_inode *ni, struct ntfs_sb_info *sbi, >> >> out: >> fnd_put(fnd); >> - >> -out1: >> return err; >> } >> diff --git a/fs/ntfs3/inode.c b/fs/ntfs3/inode.c >> index b86ec7dd731c..8f72066b3229 100644 >> --- a/fs/ntfs3/inode.c >> +++ b/fs/ntfs3/inode.c >> @@ -399,6 +399,12 @@ static struct inode *ntfs_read_mft(struct inode *inode, >> goto out; >> } >> >> + if (names != le16_to_cpu(rec->hard_links)) { >> + /* Correct minor error on the fly. Do not mark inode as dirty. */ >> + rec->hard_links = cpu_to_le16(names); >> + ni->mi.dirty = true; >> + } >> + >> set_nlink(inode, names); >> >> if (S_ISDIR(mode)) { >> @@ -1279,6 +1285,7 @@ struct inode *ntfs_create_inode(struct user_namespace *mnt_userns, >> } >> inode = &ni->vfs_inode; >> inode_init_owner(mnt_userns, inode, dir, mode); >> + mode = inode->i_mode; >> >> inode->i_atime = inode->i_mtime = inode->i_ctime = ni->i_crtime = >> current_time(inode); >> @@ -1371,6 +1378,7 @@ struct inode *ntfs_create_inode(struct user_namespace *mnt_userns, >> attr = Add2Ptr(attr, asize); >> } >> >> + attr->id = cpu_to_le16(aid++); >> if (fa & FILE_ATTRIBUTE_DIRECTORY) { >> /* >> * Regular directory or symlink to directory. >> @@ -1381,7 +1389,6 @@ struct inode *ntfs_create_inode(struct user_namespace *mnt_userns, >> >> attr->type = ATTR_ROOT; >> attr->size = cpu_to_le32(asize); >> - attr->id = cpu_to_le16(aid++); >> >> attr->name_len = ARRAY_SIZE(I30_NAME); >> attr->name_off = SIZEOF_RESIDENT_LE; >> @@ -1412,52 +1419,46 @@ struct inode *ntfs_create_inode(struct user_namespace *mnt_userns, >> /* Insert empty ATTR_DATA */ >> attr->type = ATTR_DATA; >> attr->size = cpu_to_le32(SIZEOF_RESIDENT); >> - attr->id = cpu_to_le16(aid++); >> attr->name_off = SIZEOF_RESIDENT_LE; >> attr->res.data_off = SIZEOF_RESIDENT_LE; >> - } else { >> + } else if (S_ISREG(mode)) { > > This should definetly be own patch. Way too much noice. And yes I think > this is a good change but if this is in this patch then how do we see > what is relevant? > >> /* >> - * Regular file or node. >> + * Regular file. Create empty non resident data attribute. >> */ >> attr->type = ATTR_DATA; >> - attr->id = cpu_to_le16(aid++); >> - >> - if (S_ISREG(mode)) { >> - /* Create empty non resident data attribute. */ >> - attr->non_res = 1; >> - attr->nres.evcn = cpu_to_le64(-1ll); >> - if (fa & FILE_ATTRIBUTE_SPARSE_FILE) { >> - attr->size = >> - cpu_to_le32(SIZEOF_NONRESIDENT_EX + 8); >> - attr->name_off = SIZEOF_NONRESIDENT_EX_LE; >> - attr->flags = ATTR_FLAG_SPARSED; >> - asize = SIZEOF_NONRESIDENT_EX + 8; >> - } else if (fa & FILE_ATTRIBUTE_COMPRESSED) { >> - attr->size = >> - cpu_to_le32(SIZEOF_NONRESIDENT_EX + 8); >> - attr->name_off = SIZEOF_NONRESIDENT_EX_LE; >> - attr->flags = ATTR_FLAG_COMPRESSED; >> - attr->nres.c_unit = COMPRESSION_UNIT; >> - asize = SIZEOF_NONRESIDENT_EX + 8; >> - } else { >> - attr->size = >> - cpu_to_le32(SIZEOF_NONRESIDENT + 8); >> - attr->name_off = SIZEOF_NONRESIDENT_LE; >> - asize = SIZEOF_NONRESIDENT + 8; >> - } >> - attr->nres.run_off = attr->name_off; >> + attr->non_res = 1; >> + attr->nres.evcn = cpu_to_le64(-1ll); >> + if (fa & FILE_ATTRIBUTE_SPARSE_FILE) { >> + attr->size = cpu_to_le32(SIZEOF_NONRESIDENT_EX + 8); >> + attr->name_off = SIZEOF_NONRESIDENT_EX_LE; >> + attr->flags = ATTR_FLAG_SPARSED; >> + asize = SIZEOF_NONRESIDENT_EX + 8; >> + } else if (fa & FILE_ATTRIBUTE_COMPRESSED) { >> + attr->size = cpu_to_le32(SIZEOF_NONRESIDENT_EX + 8); >> + attr->name_off = SIZEOF_NONRESIDENT_EX_LE; >> + attr->flags = ATTR_FLAG_COMPRESSED; >> + attr->nres.c_unit = COMPRESSION_UNIT; >> + asize = SIZEOF_NONRESIDENT_EX + 8; >> } else { >> - /* Create empty resident data attribute. */ >> - attr->size = cpu_to_le32(SIZEOF_RESIDENT); >> - attr->name_off = SIZEOF_RESIDENT_LE; >> - if (fa & FILE_ATTRIBUTE_SPARSE_FILE) >> - attr->flags = ATTR_FLAG_SPARSED; >> - else if (fa & FILE_ATTRIBUTE_COMPRESSED) >> - attr->flags = ATTR_FLAG_COMPRESSED; >> - attr->res.data_off = SIZEOF_RESIDENT_LE; >> - asize = SIZEOF_RESIDENT; >> - ni->ni_flags |= NI_FLAG_RESIDENT; >> + attr->size = cpu_to_le32(SIZEOF_NONRESIDENT + 8); >> + attr->name_off = SIZEOF_NONRESIDENT_LE; >> + asize = SIZEOF_NONRESIDENT + 8; >> } >> + attr->nres.run_off = attr->name_off; >> + } else { >> + /* >> + * Node. Create empty resident data attribute. >> + */ >> + attr->type = ATTR_DATA; >> + attr->size = cpu_to_le32(SIZEOF_RESIDENT); >> + attr->name_off = SIZEOF_RESIDENT_LE; >> + if (fa & FILE_ATTRIBUTE_SPARSE_FILE) >> + attr->flags = ATTR_FLAG_SPARSED; >> + else if (fa & FILE_ATTRIBUTE_COMPRESSED) >> + attr->flags = ATTR_FLAG_COMPRESSED; >> + attr->res.data_off = SIZEOF_RESIDENT_LE; >> + asize = SIZEOF_RESIDENT; >> + ni->ni_flags |= NI_FLAG_RESIDENT; >> } >> >> if (S_ISDIR(mode)) { >> @@ -1485,7 +1486,8 @@ struct inode *ntfs_create_inode(struct user_namespace *mnt_userns, >> asize = ALIGN(SIZEOF_RESIDENT + nsize, 8); >> t16 = PtrOffset(rec, attr); >> >> - if (asize + t16 + 8 > sbi->record_size) { >> + /* 0x78 - the size of EA + EAINFO to store WSL */ >> + if (asize + t16 + 0x78 + 8 > sbi->record_size) { >> CLST alen; >> CLST clst = bytes_to_cluster(sbi, nsize); >> >> @@ -1545,20 +1547,15 @@ struct inode *ntfs_create_inode(struct user_namespace *mnt_userns, >> rec->next_attr_id = cpu_to_le16(aid); >> >> /* Step 2: Add new name in index. */ >> - err = indx_insert_entry(&dir_ni->dir, dir_ni, new_de, sbi, fnd); >> + err = indx_insert_entry(&dir_ni->dir, dir_ni, new_de, sbi, fnd, 0); >> if (err) >> goto out6; >> >> - /* Update current directory record. */ >> - mark_inode_dirty(dir); >> - > > This was also probably just fix it is own? > >> inode->i_generation = le16_to_cpu(rec->seq); >> >> dir->i_mtime = dir->i_ctime = inode->i_atime; >> >> if (S_ISDIR(mode)) { >> - if (dir->i_mode & S_ISGID) >> - mode |= S_ISGID; > > This also is not relevant for this patch. Yes dead code but not releted > to this one. This kind of stuff takes lot of reviewers time. > > > I do not even feel like i will continue this review. There is just way > too much stuff and would definetly nack. But as this already is in ntfs3 > master I just don't know what to do. I cc fsdevel so maybe they can help > a little bit. > > Argillander > >> inode->i_op = &ntfs_dir_inode_operations; >> inode->i_fop = &ntfs_dir_operations; >> } else if (S_ISLNK(mode)) { >> @@ -1601,8 +1598,8 @@ struct inode *ntfs_create_inode(struct user_namespace *mnt_userns, >> d_instantiate(dentry, inode); >> >> ntfs_save_wsl_perm(inode); >> - mark_inode_dirty(inode); >> mark_inode_dirty(dir); >> + mark_inode_dirty(inode); >> >> /* Normal exit. */ >> goto out2; >> @@ -1646,61 +1643,36 @@ struct inode *ntfs_create_inode(struct user_namespace *mnt_userns, >> int ntfs_link_inode(struct inode *inode, struct dentry *dentry) >> { >> int err; >> - struct inode *dir = d_inode(dentry->d_parent); >> - struct ntfs_inode *dir_ni = ntfs_i(dir); >> struct ntfs_inode *ni = ntfs_i(inode); >> - struct super_block *sb = inode->i_sb; >> - struct ntfs_sb_info *sbi = sb->s_fs_info; >> - const struct qstr *name = &dentry->d_name; >> - struct NTFS_DE *new_de = NULL; >> - struct ATTR_FILE_NAME *fname; >> - struct ATTRIB *attr; >> - u16 key_size; >> - struct INDEX_ROOT *dir_root; >> - >> - dir_root = indx_get_root(&dir_ni->dir, dir_ni, NULL, NULL); >> - if (!dir_root) >> - return -EINVAL; >> + struct ntfs_sb_info *sbi = inode->i_sb->s_fs_info; >> + struct NTFS_DE *de; >> + struct ATTR_FILE_NAME *de_name; >> >> /* Allocate PATH_MAX bytes. */ >> - new_de = __getname(); >> - if (!new_de) >> + de = __getname(); >> + if (!de) >> return -ENOMEM; >> >> - /* Mark rw ntfs as dirty. It will be cleared at umount. */ >> - ntfs_set_state(ni->mi.sbi, NTFS_DIRTY_DIRTY); >> - >> - /* Insert file name. */ >> - err = fill_name_de(sbi, new_de, name, NULL); >> - if (err) >> - goto out; >> - >> - key_size = le16_to_cpu(new_de->key_size); >> - err = ni_insert_resident(ni, key_size, ATTR_NAME, NULL, 0, &attr, NULL); >> - if (err) >> - goto out; >> - >> - mi_get_ref(&ni->mi, &new_de->ref); >> - >> - fname = (struct ATTR_FILE_NAME *)(new_de + 1); >> - mi_get_ref(&dir_ni->mi, &fname->home); >> - fname->dup.cr_time = fname->dup.m_time = fname->dup.c_time = >> - fname->dup.a_time = kernel2nt(&inode->i_ctime); >> - fname->dup.alloc_size = fname->dup.data_size = 0; >> - fname->dup.fa = ni->std_fa; >> - fname->dup.ea_size = fname->dup.reparse = 0; >> - >> - memcpy(Add2Ptr(attr, SIZEOF_RESIDENT), fname, key_size); >> + /* Mark rw ntfs as dirty. It will be cleared at umount. */ >> + ntfs_set_state(sbi, NTFS_DIRTY_DIRTY); >> >> - err = indx_insert_entry(&dir_ni->dir, dir_ni, new_de, sbi, NULL); >> + /* Construct 'de'. */ >> + err = fill_name_de(sbi, de, &dentry->d_name, NULL); >> if (err) >> goto out; >> >> - le16_add_cpu(&ni->mi.mrec->hard_links, 1); >> - ni->mi.dirty = true; >> + de_name = (struct ATTR_FILE_NAME *)(de + 1); >> + /* Fill duplicate info. */ >> + de_name->dup.cr_time = de_name->dup.m_time = de_name->dup.c_time = >> + de_name->dup.a_time = kernel2nt(&inode->i_ctime); >> + de_name->dup.alloc_size = de_name->dup.data_size = >> + cpu_to_le64(inode->i_size); >> + de_name->dup.fa = ni->std_fa; >> + de_name->dup.ea_size = de_name->dup.reparse = 0; >> >> + err = ni_add_name(ntfs_i(d_inode(dentry->d_parent)), ni, de); >> out: >> - __putname(new_de); >> + __putname(de); >> return err; >> } >> >> @@ -1713,113 +1685,56 @@ int ntfs_link_inode(struct inode *inode, struct dentry *dentry) >> int ntfs_unlink_inode(struct inode *dir, const struct dentry *dentry) >> { >> int err; >> - struct super_block *sb = dir->i_sb; >> - struct ntfs_sb_info *sbi = sb->s_fs_info; >> + struct ntfs_sb_info *sbi = dir->i_sb->s_fs_info; >> struct inode *inode = d_inode(dentry); >> struct ntfs_inode *ni = ntfs_i(inode); >> - const struct qstr *name = &dentry->d_name; >> struct ntfs_inode *dir_ni = ntfs_i(dir); >> - struct ntfs_index *indx = &dir_ni->dir; >> - struct cpu_str *uni = NULL; >> - struct ATTR_FILE_NAME *fname; >> - u8 name_type; >> - struct ATTR_LIST_ENTRY *le; >> - struct MFT_REF ref; >> - bool is_dir = S_ISDIR(inode->i_mode); >> - struct INDEX_ROOT *dir_root; >> + struct NTFS_DE *de, *de2 = NULL; >> + int undo_remove; >> >> - dir_root = indx_get_root(indx, dir_ni, NULL, NULL); >> - if (!dir_root) >> + if (ntfs_is_meta_file(sbi, ni->mi.rno)) >> return -EINVAL; >> >> + /* Allocate PATH_MAX bytes. */ >> + de = __getname(); >> + if (!de) >> + return -ENOMEM; >> + >> ni_lock(ni); >> >> - if (is_dir && !dir_is_empty(inode)) { >> + if (S_ISDIR(inode->i_mode) && !dir_is_empty(inode)) { >> err = -ENOTEMPTY; >> - goto out1; >> - } >> - >> - if (ntfs_is_meta_file(sbi, inode->i_ino)) { >> - err = -EINVAL; >> - goto out1; >> - } >> - >> - /* Allocate PATH_MAX bytes. */ >> - uni = __getname(); >> - if (!uni) { >> - err = -ENOMEM; >> - goto out1; >> + goto out; >> } >> >> - /* Convert input string to unicode. */ >> - err = ntfs_nls_to_utf16(sbi, name->name, name->len, uni, NTFS_NAME_LEN, >> - UTF16_HOST_ENDIAN); >> + err = fill_name_de(sbi, de, &dentry->d_name, NULL); >> if (err < 0) >> - goto out2; >> - >> - /* Mark rw ntfs as dirty. It will be cleared at umount. */ >> - ntfs_set_state(sbi, NTFS_DIRTY_DIRTY); >> - >> - /* Find name in record. */ >> - mi_get_ref(&dir_ni->mi, &ref); >> - >> - le = NULL; >> - fname = ni_fname_name(ni, uni, &ref, &le); >> - if (!fname) { >> - err = -ENOENT; >> - goto out3; >> - } >> - >> - name_type = paired_name(fname->type); >> - >> - err = indx_delete_entry(indx, dir_ni, fname, fname_full_size(fname), >> - sbi); >> - if (err) >> - goto out3; >> - >> - /* Then remove name from MFT. */ >> - ni_remove_attr_le(ni, attr_from_name(fname), le); >> - >> - le16_add_cpu(&ni->mi.mrec->hard_links, -1); >> - ni->mi.dirty = true; >> - >> - if (name_type != FILE_NAME_POSIX) { >> - /* Now we should delete name by type. */ >> - fname = ni_fname_type(ni, name_type, &le); >> - if (fname) { >> - err = indx_delete_entry(indx, dir_ni, fname, >> - fname_full_size(fname), sbi); >> - if (err) >> - goto out3; >> + goto out; >> >> - ni_remove_attr_le(ni, attr_from_name(fname), le); >> + undo_remove = 0; >> + err = ni_remove_name(dir_ni, ni, de, &de2, &undo_remove); >> >> - le16_add_cpu(&ni->mi.mrec->hard_links, -1); >> - } >> - } >> -out3: >> - switch (err) { >> - case 0: >> + if (!err) { >> drop_nlink(inode); >> - break; >> - case -ENOTEMPTY: >> - case -ENOSPC: >> - case -EROFS: >> - break; >> - default: >> + dir->i_mtime = dir->i_ctime = current_time(dir); >> + mark_inode_dirty(dir); >> + inode->i_ctime = dir->i_ctime; >> + if (inode->i_nlink) >> + mark_inode_dirty(inode); >> + } else if (!ni_remove_name_undo(dir_ni, ni, de, de2, undo_remove)) { >> make_bad_inode(inode); >> + ntfs_inode_err(inode, "failed to undo unlink"); >> + ntfs_set_state(sbi, NTFS_DIRTY_ERROR); >> + } else { >> + if (ni_is_dirty(dir)) >> + mark_inode_dirty(dir); >> + if (ni_is_dirty(inode)) >> + mark_inode_dirty(inode); >> } >> >> - dir->i_mtime = dir->i_ctime = current_time(dir); >> - mark_inode_dirty(dir); >> - inode->i_ctime = dir->i_ctime; >> - if (inode->i_nlink) >> - mark_inode_dirty(inode); >> - >> -out2: >> - __putname(uni); >> -out1: >> +out: >> ni_unlock(ni); >> + __putname(de); >> return err; >> } >> >> diff --git a/fs/ntfs3/namei.c b/fs/ntfs3/namei.c >> index f79a399bd015..e58415d07132 100644 >> --- a/fs/ntfs3/namei.c >> +++ b/fs/ntfs3/namei.c >> @@ -152,12 +152,14 @@ static int ntfs_link(struct dentry *ode, struct inode *dir, struct dentry *de) >> if (inode != dir) >> ni_lock(ni); >> >> - dir->i_ctime = dir->i_mtime = inode->i_ctime = current_time(inode); >> inc_nlink(inode); >> ihold(inode); >> >> err = ntfs_link_inode(inode, de); >> + >> if (!err) { >> + dir->i_ctime = dir->i_mtime = inode->i_ctime = >> + current_time(dir); >> mark_inode_dirty(inode); >> mark_inode_dirty(dir); >> d_instantiate(de, inode); >> @@ -249,25 +251,26 @@ static int ntfs_rmdir(struct inode *dir, struct dentry *dentry) >> /* >> * ntfs_rename - inode_operations::rename >> */ >> -static int ntfs_rename(struct user_namespace *mnt_userns, struct inode *old_dir, >> - struct dentry *old_dentry, struct inode *new_dir, >> +static int ntfs_rename(struct user_namespace *mnt_userns, struct inode *dir, >> + struct dentry *dentry, struct inode *new_dir, >> struct dentry *new_dentry, u32 flags) >> { >> int err; >> - struct super_block *sb = old_dir->i_sb; >> + struct super_block *sb = dir->i_sb; >> struct ntfs_sb_info *sbi = sb->s_fs_info; >> - struct ntfs_inode *old_dir_ni = ntfs_i(old_dir); >> + struct ntfs_inode *dir_ni = ntfs_i(dir); >> struct ntfs_inode *new_dir_ni = ntfs_i(new_dir); >> - struct ntfs_inode *old_ni; >> - struct ATTR_FILE_NAME *old_name, *new_name, *fname; >> - u8 name_type; >> - bool is_same; >> - struct inode *old_inode, *new_inode; >> - struct NTFS_DE *old_de, *new_de; >> - struct ATTRIB *attr; >> - struct ATTR_LIST_ENTRY *le; >> - u16 new_de_key_size; >> - >> + struct inode *inode = d_inode(dentry); >> + struct ntfs_inode *ni = ntfs_i(inode); >> + struct inode *new_inode = d_inode(new_dentry); >> + struct NTFS_DE *de, *new_de; >> + bool is_same, is_bad; >> + /* >> + * de - memory of PATH_MAX bytes: >> + * [0-1024) - original name (dentry->d_name) >> + * [1024-2048) - paired to original name, usually DOS variant of dentry->d_name >> + * [2048-3072) - new name (new_dentry->d_name) >> + */ >> static_assert(SIZEOF_ATTRIBUTE_FILENAME_MAX + SIZEOF_RESIDENT < 1024); >> static_assert(SIZEOF_ATTRIBUTE_FILENAME_MAX + sizeof(struct NTFS_DE) < >> 1024); >> @@ -276,24 +279,18 @@ static int ntfs_rename(struct user_namespace *mnt_userns, struct inode *old_dir, >> if (flags & ~RENAME_NOREPLACE) >> return -EINVAL; >> >> - old_inode = d_inode(old_dentry); >> - new_inode = d_inode(new_dentry); >> - >> - old_ni = ntfs_i(old_inode); >> + is_same = dentry->d_name.len == new_dentry->d_name.len && >> + !memcmp(dentry->d_name.name, new_dentry->d_name.name, >> + dentry->d_name.len); >> >> - is_same = old_dentry->d_name.len == new_dentry->d_name.len && >> - !memcmp(old_dentry->d_name.name, new_dentry->d_name.name, >> - old_dentry->d_name.len); >> - >> - if (is_same && old_dir == new_dir) { >> + if (is_same && dir == new_dir) { >> /* Nothing to do. */ >> - err = 0; >> - goto out; >> + return 0; >> } >> >> - if (ntfs_is_meta_file(sbi, old_inode->i_ino)) { >> - err = -EINVAL; >> - goto out; >> + if (ntfs_is_meta_file(sbi, inode->i_ino)) { >> + /* Should we print an error? */ >> + return -EINVAL; >> } >> >> if (new_inode) { >> @@ -304,168 +301,61 @@ static int ntfs_rename(struct user_namespace *mnt_userns, struct inode *old_dir, >> ni_unlock(new_dir_ni); >> dput(new_dentry); >> if (err) >> - goto out; >> + return err; >> } >> >> /* Allocate PATH_MAX bytes. */ >> - old_de = __getname(); >> - if (!old_de) { >> - err = -ENOMEM; >> - goto out; >> - } >> + de = __getname(); >> + if (!de) >> + return -ENOMEM; >> >> - err = fill_name_de(sbi, old_de, &old_dentry->d_name, NULL); >> + /* Translate dentry->d_name into unicode form. */ >> + err = fill_name_de(sbi, de, &dentry->d_name, NULL); >> if (err < 0) >> - goto out1; >> - >> - old_name = (struct ATTR_FILE_NAME *)(old_de + 1); >> + goto out; >> >> if (is_same) { >> - new_de = old_de; >> + /* Reuse 'de'. */ >> + new_de = de; >> } else { >> - new_de = Add2Ptr(old_de, 1024); >> + /* Translate new_dentry->d_name into unicode form. */ >> + new_de = Add2Ptr(de, 2048); >> err = fill_name_de(sbi, new_de, &new_dentry->d_name, NULL); >> if (err < 0) >> - goto out1; >> - } >> - >> - ni_lock_dir(old_dir_ni); >> - ni_lock(old_ni); >> - >> - mi_get_ref(&old_dir_ni->mi, &old_name->home); >> - >> - /* Get pointer to file_name in MFT. */ >> - fname = ni_fname_name(old_ni, (struct cpu_str *)&old_name->name_len, >> - &old_name->home, &le); >> - if (!fname) { >> - err = -EINVAL; >> - goto out2; >> + goto out; >> } >> >> - /* Copy fname info from record into new fname. */ >> - new_name = (struct ATTR_FILE_NAME *)(new_de + 1); >> - memcpy(&new_name->dup, &fname->dup, sizeof(fname->dup)); >> - >> - name_type = paired_name(fname->type); >> - >> - /* Remove first name from directory. */ >> - err = indx_delete_entry(&old_dir_ni->dir, old_dir_ni, old_de + 1, >> - le16_to_cpu(old_de->key_size), sbi); >> - if (err) >> - goto out3; >> - >> - /* Remove first name from MFT. */ >> - err = ni_remove_attr_le(old_ni, attr_from_name(fname), le); >> - if (err) >> - goto out4; >> - >> - le16_add_cpu(&old_ni->mi.mrec->hard_links, -1); >> - old_ni->mi.dirty = true; >> - >> - if (name_type != FILE_NAME_POSIX) { >> - /* Get paired name. */ >> - fname = ni_fname_type(old_ni, name_type, &le); >> - if (fname) { >> - /* Remove second name from directory. */ >> - err = indx_delete_entry(&old_dir_ni->dir, old_dir_ni, >> - fname, fname_full_size(fname), >> - sbi); >> - if (err) >> - goto out5; >> - >> - /* Remove second name from MFT. */ >> - err = ni_remove_attr_le(old_ni, attr_from_name(fname), >> - le); >> - if (err) >> - goto out6; >> - >> - le16_add_cpu(&old_ni->mi.mrec->hard_links, -1); >> - old_ni->mi.dirty = true; >> + ni_lock_dir(dir_ni); >> + ni_lock(ni); >> + >> + is_bad = false; >> + err = ni_rename(dir_ni, new_dir_ni, ni, de, new_de, &is_bad); >> + if (is_bad) { >> + /* Restore after failed rename failed too. */ >> + make_bad_inode(inode); >> + ntfs_inode_err(inode, "failed to undo rename"); >> + ntfs_set_state(sbi, NTFS_DIRTY_ERROR); >> + } else if (!err) { >> + inode->i_ctime = dir->i_ctime = dir->i_mtime = >> + current_time(dir); >> + mark_inode_dirty(inode); >> + mark_inode_dirty(dir); >> + if (dir != new_dir) { >> + new_dir->i_mtime = new_dir->i_ctime = dir->i_ctime; >> + mark_inode_dirty(new_dir); >> } >> - } >> - >> - /* Add new name. */ >> - mi_get_ref(&old_ni->mi, &new_de->ref); >> - mi_get_ref(&ntfs_i(new_dir)->mi, &new_name->home); >> - >> - new_de_key_size = le16_to_cpu(new_de->key_size); >> - >> - /* Insert new name in MFT. */ >> - err = ni_insert_resident(old_ni, new_de_key_size, ATTR_NAME, NULL, 0, >> - &attr, NULL); >> - if (err) >> - goto out7; >> - >> - attr->res.flags = RESIDENT_FLAG_INDEXED; >> - >> - memcpy(Add2Ptr(attr, SIZEOF_RESIDENT), new_name, new_de_key_size); >> - >> - le16_add_cpu(&old_ni->mi.mrec->hard_links, 1); >> - old_ni->mi.dirty = true; >> - >> - /* Insert new name in directory. */ >> - err = indx_insert_entry(&new_dir_ni->dir, new_dir_ni, new_de, sbi, >> - NULL); >> - if (err) >> - goto out8; >> >> - if (IS_DIRSYNC(new_dir)) >> - err = ntfs_sync_inode(old_inode); >> - else >> - mark_inode_dirty(old_inode); >> + if (IS_DIRSYNC(dir)) >> + ntfs_sync_inode(dir); >> >> - old_dir->i_ctime = old_dir->i_mtime = current_time(old_dir); >> - if (IS_DIRSYNC(old_dir)) >> - (void)ntfs_sync_inode(old_dir); >> - else >> - mark_inode_dirty(old_dir); >> - >> - if (old_dir != new_dir) { >> - new_dir->i_mtime = new_dir->i_ctime = old_dir->i_ctime; >> - mark_inode_dirty(new_dir); >> - } >> - >> - if (old_inode) { >> - old_inode->i_ctime = old_dir->i_ctime; >> - mark_inode_dirty(old_inode); >> + if (IS_DIRSYNC(new_dir)) >> + ntfs_sync_inode(inode); >> } >> >> - err = 0; >> - /* Normal way* */ >> - goto out2; >> - >> -out8: >> - /* undo >> - * ni_insert_resident(old_ni, new_de_key_size, ATTR_NAME, NULL, 0, >> - * &attr, NULL); >> - */ >> - mi_remove_attr(&old_ni->mi, attr); >> -out7: >> - /* undo >> - * ni_remove_attr_le(old_ni, attr_from_name(fname), le); >> - */ >> -out6: >> - /* undo >> - * indx_delete_entry(&old_dir_ni->dir, old_dir_ni, >> - * fname, fname_full_size(fname), >> - * sbi); >> - */ >> -out5: >> - /* undo >> - * ni_remove_attr_le(old_ni, attr_from_name(fname), le); >> - */ >> -out4: >> - /* undo: >> - * indx_delete_entry(&old_dir_ni->dir, old_dir_ni, old_de + 1, >> - * old_de->key_size, NULL); >> - */ >> -out3: >> -out2: >> - ni_unlock(old_ni); >> - ni_unlock(old_dir_ni); >> -out1: >> - __putname(old_de); >> + ni_unlock(ni); >> + ni_unlock(dir_ni); >> out: >> + __putname(de); >> return err; >> } >> >> diff --git a/fs/ntfs3/ntfs_fs.h b/fs/ntfs3/ntfs_fs.h >> index 64ef92e16363..f9436cbbc347 100644 >> --- a/fs/ntfs3/ntfs_fs.h >> +++ b/fs/ntfs3/ntfs_fs.h >> @@ -478,7 +478,7 @@ struct ATTR_STD_INFO *ni_std(struct ntfs_inode *ni); >> struct ATTR_STD_INFO5 *ni_std5(struct ntfs_inode *ni); >> void ni_clear(struct ntfs_inode *ni); >> int ni_load_mi_ex(struct ntfs_inode *ni, CLST rno, struct mft_inode **mi); >> -int ni_load_mi(struct ntfs_inode *ni, struct ATTR_LIST_ENTRY *le, >> +int ni_load_mi(struct ntfs_inode *ni, const struct ATTR_LIST_ENTRY *le, >> struct mft_inode **mi); >> struct ATTRIB *ni_find_attr(struct ntfs_inode *ni, struct ATTRIB *attr, >> struct ATTR_LIST_ENTRY **entry_o, >> @@ -505,15 +505,18 @@ int ni_insert_nonresident(struct ntfs_inode *ni, enum ATTR_TYPE type, >> struct mft_inode **mi); >> int ni_insert_resident(struct ntfs_inode *ni, u32 data_size, >> enum ATTR_TYPE type, const __le16 *name, u8 name_len, >> - struct ATTRIB **new_attr, struct mft_inode **mi); >> -int ni_remove_attr_le(struct ntfs_inode *ni, struct ATTRIB *attr, >> - struct ATTR_LIST_ENTRY *le); >> + struct ATTRIB **new_attr, struct mft_inode **mi, >> + struct ATTR_LIST_ENTRY **le); >> +void ni_remove_attr_le(struct ntfs_inode *ni, struct ATTRIB *attr, >> + struct mft_inode *mi, struct ATTR_LIST_ENTRY *le); >> int ni_delete_all(struct ntfs_inode *ni); >> struct ATTR_FILE_NAME *ni_fname_name(struct ntfs_inode *ni, >> const struct cpu_str *uni, >> const struct MFT_REF *home, >> + struct mft_inode **mi, >> struct ATTR_LIST_ENTRY **entry); >> struct ATTR_FILE_NAME *ni_fname_type(struct ntfs_inode *ni, u8 name_type, >> + struct mft_inode **mi, >> struct ATTR_LIST_ENTRY **entry); >> int ni_new_attr_flags(struct ntfs_inode *ni, enum FILE_ATTRIBUTE new_fa); >> enum REPARSE_SIGN ni_parse_reparse(struct ntfs_inode *ni, struct ATTRIB *attr, >> @@ -528,6 +531,21 @@ int ni_read_frame(struct ntfs_inode *ni, u64 frame_vbo, struct page **pages, >> u32 pages_per_frame); >> int ni_write_frame(struct ntfs_inode *ni, struct page **pages, >> u32 pages_per_frame); >> +int ni_remove_name(struct ntfs_inode *dir_ni, struct ntfs_inode *ni, >> + struct NTFS_DE *de, struct NTFS_DE **de2, int *undo_step); >> + >> +bool ni_remove_name_undo(struct ntfs_inode *dir_ni, struct ntfs_inode *ni, >> + struct NTFS_DE *de, struct NTFS_DE *de2, >> + int undo_step); >> + >> +int ni_add_name(struct ntfs_inode *dir_ni, struct ntfs_inode *ni, >> + struct NTFS_DE *de); >> + >> +int ni_rename(struct ntfs_inode *dir_ni, struct ntfs_inode *new_dir_ni, >> + struct ntfs_inode *ni, struct NTFS_DE *de, struct NTFS_DE *new_de, >> + bool *is_bad); >> + >> +bool ni_is_dirty(struct inode *inode); >> >> /* Globals from fslog.c */ >> int log_replay(struct ntfs_inode *ni, bool *initialized); >> @@ -631,7 +649,7 @@ int indx_find_raw(struct ntfs_index *indx, struct ntfs_inode *ni, >> size_t *off, struct ntfs_fnd *fnd); >> int indx_insert_entry(struct ntfs_index *indx, struct ntfs_inode *ni, >> const struct NTFS_DE *new_de, const void *param, >> - struct ntfs_fnd *fnd); >> + struct ntfs_fnd *fnd, bool undo); >> int indx_delete_entry(struct ntfs_index *indx, struct ntfs_inode *ni, >> const void *key, u32 key_len, const void *param); >> int indx_update_dup(struct ntfs_inode *ni, struct ntfs_sb_info *sbi, >> @@ -694,7 +712,8 @@ struct ATTRIB *mi_insert_attr(struct mft_inode *mi, enum ATTR_TYPE type, >> const __le16 *name, u8 name_len, u32 asize, >> u16 name_off); >> >> -bool mi_remove_attr(struct mft_inode *mi, struct ATTRIB *attr); >> +bool mi_remove_attr(struct ntfs_inode *ni, struct mft_inode *mi, >> + struct ATTRIB *attr); >> bool mi_resize_attr(struct mft_inode *mi, struct ATTRIB *attr, int bytes); >> int mi_pack_runs(struct mft_inode *mi, struct ATTRIB *attr, >> struct runs_tree *run, CLST len); >> diff --git a/fs/ntfs3/record.c b/fs/ntfs3/record.c >> index d48a5e6c5045..61e3f2fb619f 100644 >> --- a/fs/ntfs3/record.c >> +++ b/fs/ntfs3/record.c >> @@ -489,7 +489,8 @@ struct ATTRIB *mi_insert_attr(struct mft_inode *mi, enum ATTR_TYPE type, >> * >> * NOTE: The source attr will point to next attribute. >> */ >> -bool mi_remove_attr(struct mft_inode *mi, struct ATTRIB *attr) >> +bool mi_remove_attr(struct ntfs_inode *ni, struct mft_inode *mi, >> + struct ATTRIB *attr) >> { >> struct MFT_REC *rec = mi->mrec; >> u32 aoff = PtrOffset(rec, attr); >> @@ -499,6 +500,11 @@ bool mi_remove_attr(struct mft_inode *mi, struct ATTRIB *attr) >> if (aoff + asize > used) >> return false; >> >> + if (ni && is_attr_indexed(attr)) { >> + le16_add_cpu(&ni->mi.mrec->hard_links, -1); >> + ni->mi.dirty = true; >> + } >> + >> used -= asize; >> memmove(attr, Add2Ptr(attr, asize), used - aoff); >> rec->used = cpu_to_le32(used); >> diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c >> index b4c921e4bc1a..22fd5eb32c5b 100644 >> --- a/fs/ntfs3/xattr.c >> +++ b/fs/ntfs3/xattr.c >> @@ -395,11 +395,13 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name, >> } >> >> err = ni_insert_resident(ni, sizeof(struct EA_INFO), >> - ATTR_EA_INFO, NULL, 0, NULL, NULL); >> + ATTR_EA_INFO, NULL, 0, NULL, NULL, >> + NULL); >> if (err) >> goto out; >> >> - err = ni_insert_resident(ni, 0, ATTR_EA, NULL, 0, NULL, NULL); >> + err = ni_insert_resident(ni, 0, ATTR_EA, NULL, 0, NULL, NULL, >> + NULL); >> if (err) >> goto out; >> } >> @@ -419,9 +421,7 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name, >> >> if (!size) { >> /* Delete xattr, ATTR_EA_INFO */ >> - err = ni_remove_attr_le(ni, attr, le); >> - if (err) >> - goto out; >> + ni_remove_attr_le(ni, attr, mi, le); >> } else { >> p = resident_data_ex(attr, sizeof(struct EA_INFO)); >> if (!p) { >> @@ -441,9 +441,7 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name, >> >> if (!size) { >> /* Delete xattr, ATTR_EA */ >> - err = ni_remove_attr_le(ni, attr, le); >> - if (err) >> - goto out; >> + ni_remove_attr_le(ni, attr, mi, le); >> } else if (attr->non_res) { >> err = ntfs_sb_write_run(sbi, &ea_run, 0, ea_all, size); >> if (err) >> @@ -605,8 +603,7 @@ static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns, >> goto out; >> } >> >> - err = ntfs_set_ea(inode, name, name_len, value, size, >> - acl ? 0 : XATTR_REPLACE, locked); >> + err = ntfs_set_ea(inode, name, name_len, value, size, 0, locked); >> if (!err) >> set_cached_acl(inode, type, acl); >> >> @@ -632,8 +629,10 @@ static int ntfs_xattr_get_acl(struct user_namespace *mnt_userns, >> struct posix_acl *acl; >> int err; >> >> - if (!(inode->i_sb->s_flags & SB_POSIXACL)) >> + if (!(inode->i_sb->s_flags & SB_POSIXACL)) { >> + ntfs_inode_warn(inode, "add mount option \"acl\" to use acl"); >> return -EOPNOTSUPP; >> + } >> >> acl = ntfs_get_acl(inode, type); >> if (IS_ERR(acl)) >> @@ -655,8 +654,10 @@ static int ntfs_xattr_set_acl(struct user_namespace *mnt_userns, >> struct posix_acl *acl; >> int err; >> >> - if (!(inode->i_sb->s_flags & SB_POSIXACL)) >> + if (!(inode->i_sb->s_flags & SB_POSIXACL)) { >> + ntfs_inode_warn(inode, "add mount option \"acl\" to use acl"); >> return -EOPNOTSUPP; >> + } >> >> if (!inode_owner_or_capable(mnt_userns, inode)) >> return -EPERM; >> -- >> 2.28.0 >> ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] fs/ntfs3: Rework file operations 2021-09-01 16:54 ` Konstantin Komarov @ 2021-09-02 8:25 ` Kari Argillander 0 siblings, 0 replies; 3+ messages in thread From: Kari Argillander @ 2021-09-02 8:25 UTC (permalink / raw) To: Konstantin Komarov; +Cc: ntfs3, linux-kernel, linux-fsdevel On Wed, Sep 01, 2021 at 07:54:18PM +0300, Konstantin Komarov wrote: > > > On 01.09.2021 00:54, Kari Argillander wrote: > > First of this commit is already in ntfs3/master without review. Please > > do not do that. We are middle of merge window and new this kind of patch > > is in. > > > > This patch does also lot of refactoring, dead code removel, maybe some > > bug fixes and who knows what else. One patch should address only one > > thing. I did not review whole thing but some comments there. I also > > feel like this is kinda impossible to review because so much happening. > > This is over 2000 lines long patch. Do you want to review this kind of > > patches yourself? I cc also fsdevel, maybe someone can help us little > > bit. > > > > Ntfs3 had data corruption. This commit is the result of > reworking related functions. It's a pity, that this version > wasn't in initial ntfs3 patches, because this commit basically > rewrites a significant part of driver. I did also testing with this patch. generic/502 will fail with this one occasionally. It did also do this with version v27 I think, but not with v28. It is little bit hard to debug because it would not show in single test. Only with full run. It's probably because more tests are run. I did check that 502 was run in v28 also. But there is also good things. xfstests now run more tests. Did not check what case this. These where previous results: ntfs3/default: 662 tests, 43 failures, 207 skipped, 7442 seconds Failures: generic/012 generic/013 generic/016 generic/021 generic/022 generic/040 generic/041 generic/065 generic/066 generic/092 generic/094 generic/104 generic/130 generic/177 generic/225 generic/228 generic/240 generic/255 generic/258 generic/316 generic/321 generic/322 generic/335 generic/336 generic/341 generic/342 generic/343 generic/348 generic/360 generic/401 generic/423 generic/475 generic/483 generic/498 generic/510 generic/526 generic/527 generic/538 generic/551 generic/552 generic/631 generic/633 generic/640 Totals: 455 tests, 207 skipped, 43 failures, 0 errors, 7088s New results: ntfs3/default: 684 tests, 44 failures, 217 skipped, 4682 seconds Failures: generic/012 generic/013 generic/016 generic/021 generic/022 generic/040 generic/041 generic/065 generic/066 generic/092 generic/094 generic/104 generic/130 generic/177 generic/225 generic/228 generic/240 generic/255 generic/258 generic/316 generic/321 generic/322 generic/335 generic/336 generic/341 generic/342 generic/343 generic/348 generic/360 generic/401 generic/423 generic/475 generic/483 generic/498 generic/502 generic/510 generic/526 generic/527 generic/538 generic/551 generic/552 generic/631 generic/633 generic/640 Totals: 467 tests, 217 skipped, 44 failures, 0 errors, 4455s Note that v27 did: 675 tests, 64 failures, 221 skipped > From now on our commits will follow path > "devel branch => lkml => wait for feedback => master branch" I appreciate this. > > There are no plans of further refactoring of existing codebase > that can lead to another huge commit. Like I said. There can be this kind of huge refactoring, but that has to be done in police "one patch one change". Sometimes one change is "same thing with multiple place" or "multiple change in one small place". > > Thanks for review. > > > Partial review starts here: > > > > sfr@canb.auug.org.au > > ^^ > > There is this same cc. Accident? > > > > Subject: [PATCH] fs/ntfs3: Rework file operations > > > > ^^ Not very informative commit header > > > > Maybe better should be: > > Change rename logic to add new name and then remove old one > > > > > > Please also use at least checkpatch. It found three warnings with this > > patch. > > > > On Tue, Aug 31, 2021 at 07:34:28PM +0300, Konstantin Komarov wrote: > >> Rename now works "Add new name and remove old name". > >> "Remove old name and add new name" may result in bad inode > >> if we can't add new name and then can't restore (add) old name. > > > > Also little hard to read commit message. Let me help a little bit. > > Maybe something like (not checked your patch yet): > > > > Current rename logic is that we first remove old name and then add new > > name. This can result bad inode if there is situation that when we add > > new name and it fails we have no option to return old one. > > > > Change behavier so that we first try to add new name and only after that > > we delete old one. > > > >> > >> Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com> > >> --- > >> fs/ntfs3/attrib.c | 17 +-- > >> fs/ntfs3/attrlist.c | 26 ++-- > >> fs/ntfs3/frecord.c | 342 ++++++++++++++++++++++++++++++++++++-------- > >> fs/ntfs3/fsntfs.c | 87 +++++------ > >> fs/ntfs3/index.c | 45 +++--- > >> fs/ntfs3/inode.c | 275 ++++++++++++----------------------- > >> fs/ntfs3/namei.c | 236 ++++++++---------------------- > >> fs/ntfs3/ntfs_fs.h | 31 +++- > >> fs/ntfs3/record.c | 8 +- > >> fs/ntfs3/xattr.c | 25 ++-- > >> 10 files changed, 563 insertions(+), 529 deletions(-) > >> > >> diff --git a/fs/ntfs3/attrib.c b/fs/ntfs3/attrib.c > >> index 4b285f704e62..ffc323bacc9f 100644 > >> --- a/fs/ntfs3/attrib.c > >> +++ b/fs/ntfs3/attrib.c > >> @@ -218,9 +218,11 @@ int attr_allocate_clusters(struct ntfs_sb_info *sbi, struct runs_tree *run, > >> } > >> > >> out: > >> - /* Undo. */ > >> - run_deallocate_ex(sbi, run, vcn0, vcn - vcn0, NULL, false); > >> - run_truncate(run, vcn0); > >> + /* Undo 'ntfs_look_for_free_space' */ > >> + if (vcn - vcn0) { > >> + run_deallocate_ex(sbi, run, vcn0, vcn - vcn0, NULL, false); > >> + run_truncate(run, vcn0); > >> + } > > > > Can this happend for both goto out paths? If not then other should > > return err straight away. > > > >> > >> return err; > >> } > >> @@ -701,7 +703,7 @@ int attr_set_size(struct ntfs_inode *ni, enum ATTR_TYPE type, > >> * (list entry for std attribute always first). > >> * So it is safe to step back. > >> */ > >> - mi_remove_attr(mi, attr); > >> + mi_remove_attr(NULL, mi, attr); > >> > >> if (!al_remove_le(ni, le)) { > >> err = -EINVAL; > >> @@ -1004,7 +1006,7 @@ int attr_data_get_block(struct ntfs_inode *ni, CLST vcn, CLST clen, CLST *lcn, > >> end = next_svcn; > >> while (end > evcn) { > >> /* Remove segment [svcn : evcn). */ > >> - mi_remove_attr(mi, attr); > >> + mi_remove_attr(NULL, mi, attr); > >> > >> if (!al_remove_le(ni, le)) { > >> err = -EINVAL; > >> @@ -1600,7 +1602,7 @@ int attr_allocate_frame(struct ntfs_inode *ni, CLST frame, size_t compr_size, > >> end = next_svcn; > >> while (end > evcn) { > >> /* Remove segment [svcn : evcn). */ > >> - mi_remove_attr(mi, attr); > >> + mi_remove_attr(NULL, mi, attr); > >> > >> if (!al_remove_le(ni, le)) { > >> err = -EINVAL; > >> @@ -1836,13 +1838,12 @@ int attr_collapse_range(struct ntfs_inode *ni, u64 vbo, u64 bytes) > >> u16 le_sz; > >> u16 roff = le16_to_cpu(attr->nres.run_off); > >> > >> - /* run==1 means unpack and deallocate. */ > > > > What's whit this one? Old comment maybe but should not be addressed with > > this patch. > > > >> run_unpack_ex(RUN_DEALLOCATE, sbi, ni->mi.rno, svcn, > >> evcn1 - 1, svcn, Add2Ptr(attr, roff), > >> le32_to_cpu(attr->size) - roff); > >> > >> /* Delete this attribute segment. */ > >> - mi_remove_attr(mi, attr); > >> + mi_remove_attr(NULL, mi, attr); > >> if (!le) > >> break; > >> > >> diff --git a/fs/ntfs3/attrlist.c b/fs/ntfs3/attrlist.c > >> index 32ca990af64b..fa32399eb517 100644 > >> --- a/fs/ntfs3/attrlist.c > >> +++ b/fs/ntfs3/attrlist.c > >> @@ -279,7 +279,7 @@ int al_add_le(struct ntfs_inode *ni, enum ATTR_TYPE type, const __le16 *name, > >> struct ATTR_LIST_ENTRY *le; > >> size_t off; > >> u16 sz; > >> - size_t asize, new_asize; > >> + size_t asize, new_asize, old_size; > >> u64 new_size; > >> typeof(ni->attr_list) *al = &ni->attr_list; > >> > >> @@ -287,8 +287,9 @@ int al_add_le(struct ntfs_inode *ni, enum ATTR_TYPE type, const __le16 *name, > >> * Compute the size of the new 'le' > >> */ > >> sz = le_size(name_len); > >> - new_size = al->size + sz; > >> - asize = al_aligned(al->size); > >> + old_size = al->size; > >> + asize = al_aligned(old_size); > >> new_asize = al_aligned(new_size); > >> > >> /* Scan forward to the point at which the new 'le' should be inserted. */ > >> @@ -302,13 +303,14 @@ int al_add_le(struct ntfs_inode *ni, enum ATTR_TYPE type, const __le16 *name, > >> return -ENOMEM; > >> > >> memcpy(ptr, al->le, off); > >> - memcpy(Add2Ptr(ptr, off + sz), le, al->size - off); > >> + memcpy(Add2Ptr(ptr, off + sz), le, old_size - off); > >> le = Add2Ptr(ptr, off); > >> kfree(al->le); > >> al->le = ptr; > >> } else { > >> - memmove(Add2Ptr(le, sz), le, al->size - off); > >> + memmove(Add2Ptr(le, sz), le, old_size - off); > >> } > >> + *new_le = le; > >> > >> al->size = new_size; > >> > >> @@ -321,23 +323,25 @@ int al_add_le(struct ntfs_inode *ni, enum ATTR_TYPE type, const __le16 *name, > >> le->id = id; > >> memcpy(le->name, name, sizeof(short) * name_len); > >> > >> - al->dirty = true; > >> - > >> err = attr_set_size(ni, ATTR_LIST, NULL, 0, &al->run, new_size, > >> &new_size, true, &attr); > >> - if (err) > >> + if (err) { > >> + /* Undo memmove above. */ > > > > There is path also for memcpy. What if we go there. Is this path then > > impossible? Just checking. > > > >> + memmove(le, Add2Ptr(le, sz), old_size - off); > >> + al->size = old_size; > >> return err; > >> + } > >> + > >> + al->dirty = true; > >> > >> if (attr && attr->non_res) { > >> err = ntfs_sb_write_run(ni->mi.sbi, &al->run, 0, al->le, > >> al->size); > >> if (err) > >> return err; > >> + al->dirty = false; > >> } > >> > >> - al->dirty = false; > >> - *new_le = le; > >> - > >> return 0; > >> } > >> > >> diff --git a/fs/ntfs3/frecord.c b/fs/ntfs3/frecord.c > >> index 9d374827750b..3f48b612ec96 100644 > >> --- a/fs/ntfs3/frecord.c > >> +++ b/fs/ntfs3/frecord.c > >> @@ -163,7 +163,7 @@ int ni_load_mi_ex(struct ntfs_inode *ni, CLST rno, struct mft_inode **mi) > >> /* > >> * ni_load_mi - Load mft_inode corresponded list_entry. > >> */ > >> -int ni_load_mi(struct ntfs_inode *ni, struct ATTR_LIST_ENTRY *le, > >> +int ni_load_mi(struct ntfs_inode *ni, const struct ATTR_LIST_ENTRY *le, > >> struct mft_inode **mi) > > > > Feels like unnecesary change for this patch. > > > >> { > >> CLST rno; > >> @@ -402,7 +402,7 @@ int ni_remove_attr(struct ntfs_inode *ni, enum ATTR_TYPE type, > >> if (!attr) > >> return -ENOENT; > >> > >> - mi_remove_attr(&ni->mi, attr); > >> + mi_remove_attr(ni, &ni->mi, attr); > >> return 0; > >> } > >> > >> @@ -441,7 +441,7 @@ int ni_remove_attr(struct ntfs_inode *ni, enum ATTR_TYPE type, > >> if (!attr) > >> return -ENOENT; > >> > >> - mi_remove_attr(mi, attr); > >> + mi_remove_attr(ni, mi, attr); > >> > >> if (PtrOffset(ni->attr_list.le, le) >= ni->attr_list.size) > >> return 0; > >> @@ -454,12 +454,11 @@ int ni_remove_attr(struct ntfs_inode *ni, enum ATTR_TYPE type, > >> * > >> * Return: Not full constructed attribute or NULL if not possible to create. > >> */ > >> -static struct ATTRIB *ni_ins_new_attr(struct ntfs_inode *ni, > >> - struct mft_inode *mi, > >> - struct ATTR_LIST_ENTRY *le, > >> - enum ATTR_TYPE type, const __le16 *name, > >> - u8 name_len, u32 asize, u16 name_off, > >> - CLST svcn) > >> +static struct ATTRIB * > >> +ni_ins_new_attr(struct ntfs_inode *ni, struct mft_inode *mi, > >> + struct ATTR_LIST_ENTRY *le, enum ATTR_TYPE type, > >> + const __le16 *name, u8 name_len, u32 asize, u16 name_off, > >> + CLST svcn, struct ATTR_LIST_ENTRY **ins_le) > > > > Reviewing would be lot lot easier if you make one patch which just adds > > this new ins_le to every function needed. Just call it with NULL. Then > > there would be much less noise to watch. > > > >> { > >> int err; > >> struct ATTRIB *attr; > >> @@ -507,6 +506,8 @@ static struct ATTRIB *ni_ins_new_attr(struct ntfs_inode *ni, > >> le->ref = ref; > >> > >> out: > >> + if (ins_le) > >> + *ins_le = le; > >> return attr; > >> } > >> > >> @@ -599,7 +600,7 @@ static int ni_repack(struct ntfs_inode *ni) > >> if (next_svcn >= evcn + 1) { > >> /* We can remove this attribute segment. */ > >> al_remove_le(ni, le); > >> - mi_remove_attr(mi, attr); > >> + mi_remove_attr(NULL, mi, attr); > >> le = le_p; > >> continue; > >> } > >> @@ -695,8 +696,8 @@ static int ni_try_remove_attr_list(struct ntfs_inode *ni) > >> free -= asize; > >> } > >> > >> - /* Is seems that attribute list can be removed from primary record. */ > >> - mi_remove_attr(&ni->mi, attr_list); > >> + /* It seems that attribute list can be removed from primary record. */ > >> + mi_remove_attr(NULL, &ni->mi, attr_list); > >> > >> /* > >> * Repeat the cycle above and move all attributes to primary record. > >> @@ -724,7 +725,7 @@ static int ni_try_remove_attr_list(struct ntfs_inode *ni) > >> attr_ins->id = id; > >> > >> /* Remove from original record. */ > >> - mi_remove_attr(mi, attr); > >> + mi_remove_attr(NULL, mi, attr); > >> } > >> > >> run_deallocate(sbi, &ni->attr_list.run, true); > >> @@ -842,7 +843,8 @@ int ni_create_attr_list(struct ntfs_inode *ni) > >> memcpy(attr, b, asize); > >> attr->id = le_b[nb]->id; > >> > >> - WARN_ON(!mi_remove_attr(&ni->mi, b)); > >> + /* Remove from primary record. */ > >> + WARN_ON(!mi_remove_attr(NULL, &ni->mi, b)); > >> > >> if (to_free <= asize) > >> break; > >> @@ -883,7 +885,8 @@ int ni_create_attr_list(struct ntfs_inode *ni) > >> static int ni_ins_attr_ext(struct ntfs_inode *ni, struct ATTR_LIST_ENTRY *le, > >> enum ATTR_TYPE type, const __le16 *name, u8 name_len, > >> u32 asize, CLST svcn, u16 name_off, bool force_ext, > >> - struct ATTRIB **ins_attr, struct mft_inode **ins_mi) > >> + struct ATTRIB **ins_attr, struct mft_inode **ins_mi, > >> + struct ATTR_LIST_ENTRY **ins_le) > >> { > >> struct ATTRIB *attr; > >> struct mft_inode *mi; > >> @@ -956,12 +959,14 @@ static int ni_ins_attr_ext(struct ntfs_inode *ni, struct ATTR_LIST_ENTRY *le, > >> > >> /* Try to insert attribute into this subrecord. */ > >> attr = ni_ins_new_attr(ni, mi, le, type, name, name_len, asize, > >> - name_off, svcn); > >> + name_off, svcn, ins_le); > >> if (!attr) > >> continue; > >> > >> if (ins_attr) > >> *ins_attr = attr; > >> + if (ins_mi) > >> + *ins_mi = mi; > >> return 0; > >> } > >> > >> @@ -977,7 +982,7 @@ static int ni_ins_attr_ext(struct ntfs_inode *ni, struct ATTR_LIST_ENTRY *le, > >> } > >> > >> attr = ni_ins_new_attr(ni, mi, le, type, name, name_len, asize, > >> - name_off, svcn); > >> + name_off, svcn, ins_le); > >> if (!attr) > >> goto out2; > >> > >> @@ -1016,7 +1021,8 @@ static int ni_ins_attr_ext(struct ntfs_inode *ni, struct ATTR_LIST_ENTRY *le, > >> static int ni_insert_attr(struct ntfs_inode *ni, enum ATTR_TYPE type, > >> const __le16 *name, u8 name_len, u32 asize, > >> u16 name_off, CLST svcn, struct ATTRIB **ins_attr, > >> - struct mft_inode **ins_mi) > >> + struct mft_inode **ins_mi, > >> + struct ATTR_LIST_ENTRY **ins_le) > >> { > >> struct ntfs_sb_info *sbi = ni->mi.sbi; > >> int err; > >> @@ -1045,7 +1051,7 @@ static int ni_insert_attr(struct ntfs_inode *ni, enum ATTR_TYPE type, > >> > >> if (asize <= free) { > >> attr = ni_ins_new_attr(ni, &ni->mi, NULL, type, name, name_len, > >> - asize, name_off, svcn); > >> + asize, name_off, svcn, ins_le); > >> if (attr) { > >> if (ins_attr) > >> *ins_attr = attr; > >> @@ -1059,7 +1065,8 @@ static int ni_insert_attr(struct ntfs_inode *ni, enum ATTR_TYPE type, > >> if (!is_mft || type != ATTR_DATA || svcn) { > >> /* This ATTRIB will be external. */ > >> err = ni_ins_attr_ext(ni, NULL, type, name, name_len, asize, > >> - svcn, name_off, false, ins_attr, ins_mi); > >> + svcn, name_off, false, ins_attr, ins_mi, > >> + ins_le); > >> goto out; > >> } > >> > >> @@ -1117,7 +1124,7 @@ static int ni_insert_attr(struct ntfs_inode *ni, enum ATTR_TYPE type, > >> t16 = le16_to_cpu(attr->name_off); > >> err = ni_ins_attr_ext(ni, le, attr->type, Add2Ptr(attr, t16), > >> attr->name_len, t32, attr_svcn(attr), t16, > >> - false, &eattr, NULL); > >> + false, &eattr, NULL, NULL); > >> if (err) > >> return err; > >> > >> @@ -1125,8 +1132,8 @@ static int ni_insert_attr(struct ntfs_inode *ni, enum ATTR_TYPE type, > >> memcpy(eattr, attr, t32); > >> eattr->id = id; > >> > >> - /* Remove attrib from primary record. */ > >> - mi_remove_attr(&ni->mi, attr); > >> + /* Remove from primary record. */ > >> + mi_remove_attr(NULL, &ni->mi, attr); > >> > >> /* attr now points to next attribute. */ > >> if (attr->type == ATTR_END) > >> @@ -1136,7 +1143,7 @@ static int ni_insert_attr(struct ntfs_inode *ni, enum ATTR_TYPE type, > >> ; > >> > >> attr = ni_ins_new_attr(ni, &ni->mi, NULL, type, name, name_len, asize, > >> - name_off, svcn); > >> + name_off, svcn, ins_le); > >> if (!attr) { > >> err = -EINVAL; > >> goto out; > >> @@ -1251,7 +1258,7 @@ static int ni_expand_mft_list(struct ntfs_inode *ni) > >> */ > >> attr = ni_ins_new_attr(ni, mi_min, NULL, ATTR_DATA, NULL, 0, > >> SIZEOF_NONRESIDENT + run_size, > >> - SIZEOF_NONRESIDENT, svcn); > >> + SIZEOF_NONRESIDENT, svcn, NULL); > >> if (!attr) { > >> err = -EINVAL; > >> goto out; > >> @@ -1315,14 +1322,15 @@ int ni_expand_list(struct ntfs_inode *ni) > >> err = ni_ins_attr_ext(ni, le, attr->type, attr_name(attr), > >> attr->name_len, asize, attr_svcn(attr), > >> le16_to_cpu(attr->name_off), true, > >> - &ins_attr, NULL); > >> + &ins_attr, NULL, NULL); > >> > >> if (err) > >> goto out; > >> > >> memcpy(ins_attr, attr, asize); > >> ins_attr->id = le->id; > >> - mi_remove_attr(&ni->mi, attr); > >> + /* Remove from primary record. */ > >> + mi_remove_attr(NULL, &ni->mi, attr); > >> > >> done += asize; > >> goto out; > >> @@ -1382,7 +1390,7 @@ int ni_insert_nonresident(struct ntfs_inode *ni, enum ATTR_TYPE type, > >> } > >> > >> err = ni_insert_attr(ni, type, name, name_len, asize, name_off, svcn, > >> - &attr, mi); > >> + &attr, mi, NULL); > >> > >> if (err) > >> goto out; > >> @@ -1422,7 +1430,8 @@ int ni_insert_nonresident(struct ntfs_inode *ni, enum ATTR_TYPE type, > >> */ > >> int ni_insert_resident(struct ntfs_inode *ni, u32 data_size, > >> enum ATTR_TYPE type, const __le16 *name, u8 name_len, > >> - struct ATTRIB **new_attr, struct mft_inode **mi) > >> + struct ATTRIB **new_attr, struct mft_inode **mi, > >> + struct ATTR_LIST_ENTRY **le) > >> { > >> int err; > >> u32 name_size = ALIGN(name_len * sizeof(short), 8); > >> @@ -1430,7 +1439,7 @@ int ni_insert_resident(struct ntfs_inode *ni, u32 data_size, > >> struct ATTRIB *attr; > >> > >> err = ni_insert_attr(ni, type, name, name_len, asize, SIZEOF_RESIDENT, > >> - 0, &attr, mi); > >> + 0, &attr, mi, le); > >> if (err) > >> return err; > >> > >> @@ -1439,8 +1448,13 @@ int ni_insert_resident(struct ntfs_inode *ni, u32 data_size, > >> > >> attr->res.data_size = cpu_to_le32(data_size); > >> attr->res.data_off = cpu_to_le16(SIZEOF_RESIDENT + name_size); > >> - if (type == ATTR_NAME) > >> + if (type == ATTR_NAME) { > >> attr->res.flags = RESIDENT_FLAG_INDEXED; > >> + > >> + /* is_attr_indexed(attr)) == true */ > >> + le16_add_cpu(&ni->mi.mrec->hard_links, +1); > >> + ni->mi.dirty = true; > >> + } > >> attr->res.res = 0; > >> > >> if (new_attr) > >> @@ -1452,22 +1466,13 @@ int ni_insert_resident(struct ntfs_inode *ni, u32 data_size, > >> /* > >> * ni_remove_attr_le - Remove attribute from record. > >> */ > >> -int ni_remove_attr_le(struct ntfs_inode *ni, struct ATTRIB *attr, > >> - struct ATTR_LIST_ENTRY *le) > >> +void ni_remove_attr_le(struct ntfs_inode *ni, struct ATTRIB *attr, > >> + struct mft_inode *mi, struct ATTR_LIST_ENTRY *le) > >> { > >> - int err; > >> - struct mft_inode *mi; > >> - > >> - err = ni_load_mi(ni, le, &mi); > >> - if (err) > >> - return err; > >> - > >> - mi_remove_attr(mi, attr); > >> + mi_remove_attr(ni, mi, attr); > >> > >> if (le) > >> al_remove_le(ni, le); > >> - > >> - return 0; > >> } > >> > >> /* > >> @@ -1549,10 +1554,12 @@ int ni_delete_all(struct ntfs_inode *ni) > >> > >> /* ni_fname_name > >> * > >> - *Return: File name attribute by its value. */ > >> + * Return: File name attribute by its value. > >> + */ > >> struct ATTR_FILE_NAME *ni_fname_name(struct ntfs_inode *ni, > >> const struct cpu_str *uni, > >> const struct MFT_REF *home_dir, > >> + struct mft_inode **mi, > >> struct ATTR_LIST_ENTRY **le) > >> { > >> struct ATTRIB *attr = NULL; > >> @@ -1562,7 +1569,7 @@ struct ATTR_FILE_NAME *ni_fname_name(struct ntfs_inode *ni, > >> > >> /* Enumerate all names. */ > >> next: > >> - attr = ni_find_attr(ni, attr, le, ATTR_NAME, NULL, 0, NULL, NULL); > >> + attr = ni_find_attr(ni, attr, le, ATTR_NAME, NULL, 0, NULL, mi); > >> if (!attr) > >> return NULL; > >> > >> @@ -1592,6 +1599,7 @@ struct ATTR_FILE_NAME *ni_fname_name(struct ntfs_inode *ni, > >> * Return: File name attribute with given type. > >> */ > >> struct ATTR_FILE_NAME *ni_fname_type(struct ntfs_inode *ni, u8 name_type, > >> + struct mft_inode **mi, > >> struct ATTR_LIST_ENTRY **le) > >> { > >> struct ATTRIB *attr = NULL; > >> @@ -1599,10 +1607,12 @@ struct ATTR_FILE_NAME *ni_fname_type(struct ntfs_inode *ni, u8 name_type, > >> > >> *le = NULL; > >> > >> + if (FILE_NAME_POSIX == name_type) > >> + return NULL; > >> + > >> /* Enumerate all names. */ > >> for (;;) { > >> - attr = ni_find_attr(ni, attr, le, ATTR_NAME, NULL, 0, NULL, > >> - NULL); > >> + attr = ni_find_attr(ni, attr, le, ATTR_NAME, NULL, 0, NULL, mi); > >> if (!attr) > >> return NULL; > >> > >> @@ -2788,6 +2798,222 @@ int ni_write_frame(struct ntfs_inode *ni, struct page **pages, > >> return err; > >> } > >> > >> +/* > >> + * ni_remove_name - Removes name 'de' from MFT and from directory. > >> + * 'de2' and 'undo_step' are used to restore MFT/dir, if error occurs. > >> + */ > >> +int ni_remove_name(struct ntfs_inode *dir_ni, struct ntfs_inode *ni, > >> + struct NTFS_DE *de, struct NTFS_DE **de2, int *undo_step) > >> +{ > >> + int err; > >> + struct ntfs_sb_info *sbi = ni->mi.sbi; > >> + struct ATTR_FILE_NAME *de_name = (struct ATTR_FILE_NAME *)(de + 1); > >> + struct ATTR_FILE_NAME *fname; > >> + struct ATTR_LIST_ENTRY *le; > >> + struct mft_inode *mi; > >> + u16 de_key_size = le16_to_cpu(de->key_size); > >> + u8 name_type; > >> + > >> + *undo_step = 0; > > > > It looks to me that this function can make undo by itself. There is no > > need for ni_remove_name_undo. This looks very strange way to do this. > > > > One other way if really needed is also to use return value, but this > > pointer undo_step feels just wrong. > > > >> + > >> + /* Find name in record. */ > >> + mi_get_ref(&dir_ni->mi, &de_name->home); > >> + > >> + fname = ni_fname_name(ni, (struct cpu_str *)&de_name->name_len, > >> + &de_name->home, &mi, &le); > >> + if (!fname) > >> + return -ENOENT; > >> + > >> + memcpy(&de_name->dup, &fname->dup, sizeof(struct NTFS_DUP_INFO)); > >> + name_type = paired_name(fname->type); > >> + > >> + /* Mark ntfs as dirty. It will be cleared at umount. */ > >> + ntfs_set_state(sbi, NTFS_DIRTY_DIRTY); > >> + > >> + /* Step 1: Remove name from directory. */ > >> + err = indx_delete_entry(&dir_ni->dir, dir_ni, fname, de_key_size, sbi); > >> + if (err) > >> + return err; > >> + > >> + /* Step 2: Remove name from MFT. */ > >> + ni_remove_attr_le(ni, attr_from_name(fname), mi, le); > >> + > >> + *undo_step = 2; > >> + > >> + /* Get paired name. */ > >> + fname = ni_fname_type(ni, name_type, &mi, &le); > > > > It does not feel right that ni_fname_type does not return anything if > > name_type is FILE_NAME_POSIX. It can do it right? Why won't we check > > before ni_fname_type call if name_type == FILE_NAME_POSIX and just exit > > with return 0 if it is. > > > >> + if (fname) { > >> + u16 de2_key_size = fname_full_size(fname); > >> + > >> + *de2 = Add2Ptr(de, 1024); > >> + (*de2)->key_size = cpu_to_le16(de2_key_size); > >> + > >> + memcpy(*de2 + 1, fname, de2_key_size); > >> + > >> + /* Step 3: Remove paired name from directory. */ > >> + err = indx_delete_entry(&dir_ni->dir, dir_ni, fname, > >> + de2_key_size, sbi); > >> + if (err) > >> + return err; > >> + > >> + /* Step 4: Remove paired name from MFT. */ > >> + ni_remove_attr_le(ni, attr_from_name(fname), mi, le); > >> + > >> + *undo_step = 4; > >> + } > >> + return 0; > >> +} > >> + > >> +/* > >> + * ni_remove_name_undo - Paired function for ni_remove_name. > >> + * > >> + * Return: True if ok > >> + */ > >> +bool ni_remove_name_undo(struct ntfs_inode *dir_ni, struct ntfs_inode *ni, > >> + struct NTFS_DE *de, struct NTFS_DE *de2, int undo_step) > >> +{ > >> + struct ntfs_sb_info *sbi = ni->mi.sbi; > >> + struct ATTRIB *attr; > >> + u16 de_key_size = de2 ? le16_to_cpu(de2->key_size) : 0; > >> + > >> + switch (undo_step) { > >> + case 4: > >> + if (ni_insert_resident(ni, de_key_size, ATTR_NAME, NULL, 0, > >> + &attr, NULL, NULL)) { > >> + return false; > >> + } > >> + memcpy(Add2Ptr(attr, SIZEOF_RESIDENT), de2 + 1, de_key_size); > >> + > >> + mi_get_ref(&ni->mi, &de2->ref); > >> + de2->size = cpu_to_le16(ALIGN(de_key_size, 8) + > >> + sizeof(struct NTFS_DE)); > >> + de2->flags = 0; > >> + de2->res = 0; > >> + > >> + if (indx_insert_entry(&dir_ni->dir, dir_ni, de2, sbi, NULL, > >> + 1)) { > >> + return false; > >> + } > >> + fallthrough; > >> + > >> + case 2: > >> + de_key_size = le16_to_cpu(de->key_size); > >> + > >> + if (ni_insert_resident(ni, de_key_size, ATTR_NAME, NULL, 0, > >> + &attr, NULL, NULL)) { > >> + return false; > >> + } > >> + > >> + memcpy(Add2Ptr(attr, SIZEOF_RESIDENT), de + 1, de_key_size); > >> + mi_get_ref(&ni->mi, &de->ref); > >> + > >> + if (indx_insert_entry(&dir_ni->dir, dir_ni, de, sbi, NULL, 1)) { > >> + return false; > >> + } > >> + } > >> + > >> + return true; > >> +} > >> + > >> +/* > >> + * ni_add_name - Add new name in MFT and in directory. > > > > *to would be better? Also in comments. > > > >> + */ > >> +int ni_add_name(struct ntfs_inode *dir_ni, struct ntfs_inode *ni, > >> + struct NTFS_DE *de) > >> +{ > >> + int err; > >> + struct ATTRIB *attr; > >> + struct ATTR_LIST_ENTRY *le; > >> + struct mft_inode *mi; > >> + struct ATTR_FILE_NAME *de_name = (struct ATTR_FILE_NAME *)(de + 1); > >> + u16 de_key_size = le16_to_cpu(de->key_size); > >> + > >> + mi_get_ref(&ni->mi, &de->ref); > >> + mi_get_ref(&dir_ni->mi, &de_name->home); > >> + > >> + /* Insert new name in MFT. */ > >> + err = ni_insert_resident(ni, de_key_size, ATTR_NAME, NULL, 0, &attr, > >> + &mi, &le); > >> + if (err) > >> + return err; > >> + > >> + memcpy(Add2Ptr(attr, SIZEOF_RESIDENT), de_name, de_key_size); > >> + > >> + /* Insert new name in directory. */ > >> + err = indx_insert_entry(&dir_ni->dir, dir_ni, de, ni->mi.sbi, NULL, 0); > >> + if (err) > >> + ni_remove_attr_le(ni, attr, mi, le); > >> + > >> + return err; > >> +} > >> + > >> +/* > >> + * ni_rename - Remove one name and insert new name. > >> + */ > >> +int ni_rename(struct ntfs_inode *dir_ni, struct ntfs_inode *new_dir_ni, > >> + struct ntfs_inode *ni, struct NTFS_DE *de, struct NTFS_DE *new_de, > >> + bool *is_bad) > >> +{ > >> + int err; > >> + struct NTFS_DE *de2 = NULL; > >> + int undo = 0; > >> + > >> + /* > >> + * There are two possible ways to rename: > >> + * 1) Add new name and remove old name. > >> + * 2) Remove old name and add new name. > >> + * > >> + * In most cases (not all!) adding new name in MFT and in directory can > >> + * allocate additional cluster(s). > >> + * Second way may result to bad inode if we can't add new name > >> + * and then can't restore (add) old name. > >> + */ > >> + > >> + /* > >> + * Way 1 - Add new + remove old. > >> + */ > > > > Way too big comment for thing this small. > > > >> + err = ni_add_name(new_dir_ni, ni, new_de); > > > > if (err) > > return err; > > > >> + if (!err) { > >> + err = ni_remove_name(dir_ni, ni, de, &de2, &undo); > >> + if (err && ni_remove_name(new_dir_ni, ni, new_de, &de2, &undo)) > >> + *is_bad = true; > > > > Maybe we can return errno or 1 here. is_bad is again imo little > > unnecessary here. And now that I think this maybe this should be in > > caller site in ntfs_rename. > > > >> + } > >> + > >> + /* > >> + * Way 2 - Remove old + add new. > >> + */ > >> + /* > >> + * err = ni_remove_name(dir_ni, ni, de, &de2, &undo); > >> + * if (!err) { > >> + * err = ni_add_name(new_dir_ni, ni, new_de); > >> + * if (err && !ni_remove_name_undo(dir_ni, ni, de, de2, undo)) > >> + * *is_bad = true; > >> + * } > >> + */ > >> + > >> + return err; > >> +} > >> + > >> +/* > >> + * ni_is_dirty - Return: True if 'ni' requires ni_write_inode. > >> + */ > >> +bool ni_is_dirty(struct inode *inode) > >> +{ > >> + struct ntfs_inode *ni = ntfs_i(inode); > >> + struct rb_node *node; > >> + > >> + if (ni->mi.dirty || ni->attr_list.dirty || > >> + (ni->ni_flags & NI_FLAG_UPDATE_PARENT)) > > > > No need to save space here. Just another if statment for third one. > > > >> + return true; > >> + > >> + for (node = rb_first(&ni->mi_tree); node; node = rb_next(node)) { > >> + if (rb_entry(node, struct mft_inode, node)->dirty) > >> + return true; > >> + } > >> + > >> + return false; > >> +} > >> + > >> /* > >> * ni_update_parent > >> * > >> @@ -2802,8 +3028,6 @@ static bool ni_update_parent(struct ntfs_inode *ni, struct NTFS_DUP_INFO *dup, > >> struct ntfs_sb_info *sbi = ni->mi.sbi; > >> struct super_block *sb = sbi->sb; > >> bool re_dirty = false; > >> - bool active = sb->s_flags & SB_ACTIVE; > >> - bool upd_parent = ni->ni_flags & NI_FLAG_UPDATE_PARENT; > >> > >> if (ni->mi.mrec->flags & RECORD_FLAG_DIR) { > >> dup->fa |= FILE_ATTRIBUTE_DIRECTORY; > >> @@ -2867,19 +3091,9 @@ static bool ni_update_parent(struct ntfs_inode *ni, struct NTFS_DUP_INFO *dup, > >> struct ATTR_FILE_NAME *fname; > >> > >> fname = resident_data_ex(attr, SIZEOF_ATTRIBUTE_FILENAME); > >> - if (!fname) > >> + if (!fname || !memcmp(&fname->dup, dup, sizeof(fname->dup))) > >> continue; > >> > >> - if (memcmp(&fname->dup, dup, sizeof(fname->dup))) { > >> - memcpy(&fname->dup, dup, sizeof(fname->dup)); > >> - mi->dirty = true; > >> - } else if (!upd_parent) { > >> - continue; > >> - } > >> - > >> - if (!active) > >> - continue; /* Avoid __wait_on_freeing_inode(inode); */ > >> - > >> /* ntfs_iget5 may sleep. */ > >> dir = ntfs_iget5(sb, &fname->home, NULL); > >> if (IS_ERR(dir)) { > >> @@ -2898,6 +3112,8 @@ static bool ni_update_parent(struct ntfs_inode *ni, struct NTFS_DUP_INFO *dup, > >> } else { > >> indx_update_dup(dir_ni, sbi, fname, dup, sync); > >> ni_unlock(dir_ni); > >> + memcpy(&fname->dup, dup, sizeof(fname->dup)); > >> + mi->dirty = true; > >> } > >> } > >> iput(dir); > >> @@ -2969,7 +3185,9 @@ int ni_write_inode(struct inode *inode, int sync, const char *hint) > >> ni->mi.dirty = true; > >> > >> if (!ntfs_is_meta_file(sbi, inode->i_ino) && > >> - (modified || (ni->ni_flags & NI_FLAG_UPDATE_PARENT))) { > >> + (modified || (ni->ni_flags & NI_FLAG_UPDATE_PARENT)) > >> + /* Avoid __wait_on_freeing_inode(inode). */ > >> + && (sb->s_flags & SB_ACTIVE)) { > >> dup.cr_time = std->cr_time; > >> /* Not critical if this function fail. */ > >> re_dirty = ni_update_parent(ni, &dup, sync); > >> @@ -3033,7 +3251,7 @@ int ni_write_inode(struct inode *inode, int sync, const char *hint) > >> return err; > >> } > >> > >> - if (re_dirty && (sb->s_flags & SB_ACTIVE)) > >> + if (re_dirty) > >> mark_inode_dirty_sync(inode); > >> > >> return 0; > >> diff --git a/fs/ntfs3/fsntfs.c b/fs/ntfs3/fsntfs.c > >> index 0edb95ed9717..669249439217 100644 > >> --- a/fs/ntfs3/fsntfs.c > >> +++ b/fs/ntfs3/fsntfs.c > >> @@ -358,29 +358,25 @@ int ntfs_look_for_free_space(struct ntfs_sb_info *sbi, CLST lcn, CLST len, > >> enum ALLOCATE_OPT opt) > >> { > >> int err; > >> + CLST alen = 0; > >> struct super_block *sb = sbi->sb; > >> - size_t a_lcn, zlen, zeroes, zlcn, zlen2, ztrim, new_zlen; > >> + size_t alcn, zlen, zeroes, zlcn, zlen2, ztrim, new_zlen; > >> struct wnd_bitmap *wnd = &sbi->used.bitmap; > >> > >> down_write_nested(&wnd->rw_lock, BITMAP_MUTEX_CLUSTERS); > >> if (opt & ALLOCATE_MFT) { > >> - CLST alen; > >> - > >> zlen = wnd_zone_len(wnd); > >> > >> if (!zlen) { > >> err = ntfs_refresh_zone(sbi); > >> if (err) > >> goto out; > > > > Before we return this error code. Now we return -ENOSPC if this error. > > > >> - > >> zlen = wnd_zone_len(wnd); > >> + } > >> > >> - if (!zlen) { > >> - ntfs_err(sbi->sb, > >> - "no free space to extend mft"); > >> - err = -ENOSPC; > >> - goto out; > >> - } > >> + if (!zlen) { > >> + ntfs_err(sbi->sb, "no free space to extend mft"); > >> + goto out; > > > > This is just refactoring and should do in different patch. > > > >> } > >> > >> lcn = wnd_zone_bit(wnd); > >> @@ -389,14 +385,13 @@ int ntfs_look_for_free_space(struct ntfs_sb_info *sbi, CLST lcn, CLST len, > >> wnd_zone_set(wnd, lcn + alen, zlen - alen); > >> > >> err = wnd_set_used(wnd, lcn, alen); > >> - if (err) > >> - goto out; > >> - > >> - *new_lcn = lcn; > >> - *new_len = alen; > >> - goto ok; > >> + if (err) { > >> + up_write(&wnd->rw_lock); > >> + return err; > >> + } > >> + alcn = lcn; > >> + goto out; > >> } > >> - > >> /* > >> * 'Cause cluster 0 is always used this value means that we should use > >> * cached value of 'next_free_lcn' to improve performance. > >> @@ -407,22 +402,17 @@ int ntfs_look_for_free_space(struct ntfs_sb_info *sbi, CLST lcn, CLST len, > >> if (lcn >= wnd->nbits) > >> lcn = 0; > >> > >> - *new_len = wnd_find(wnd, len, lcn, BITMAP_FIND_MARK_AS_USED, &a_lcn); > >> - if (*new_len) { > >> - *new_lcn = a_lcn; > >> - goto ok; > >> - } > >> + alen = wnd_find(wnd, len, lcn, BITMAP_FIND_MARK_AS_USED, &alcn); > >> + if (alen) > >> + goto out; > >> > >> /* Try to use clusters from MftZone. */ > >> zlen = wnd_zone_len(wnd); > >> zeroes = wnd_zeroes(wnd); > >> > >> - /* Check too big request. */ > >> - if (len > zeroes + zlen) > >> - goto no_space; > >> - > >> - if (zlen <= NTFS_MIN_MFT_ZONE) > >> - goto no_space; > >> + /* Check too big request */ > >> + if (len > zeroes + zlen || zlen <= NTFS_MIN_MFT_ZONE) > >> + goto out; > >> > >> /* How many clusters to cat from zone. */ > >> zlcn = wnd_zone_bit(wnd); > >> @@ -439,31 +429,24 @@ int ntfs_look_for_free_space(struct ntfs_sb_info *sbi, CLST lcn, CLST len, > >> wnd_zone_set(wnd, zlcn, new_zlen); > >> > >> /* Allocate continues clusters. */ > >> - *new_len = > >> - wnd_find(wnd, len, 0, > >> - BITMAP_FIND_MARK_AS_USED | BITMAP_FIND_FULL, &a_lcn); > >> - if (*new_len) { > >> - *new_lcn = a_lcn; > >> - goto ok; > >> - } > >> - > >> -no_space: > >> - up_write(&wnd->rw_lock); > >> - > >> - return -ENOSPC; > >> - > >> -ok: > >> - err = 0; > >> + alen = wnd_find(wnd, len, 0, > >> + BITMAP_FIND_MARK_AS_USED | BITMAP_FIND_FULL, &alcn); > >> > >> - ntfs_unmap_meta(sb, *new_lcn, *new_len); > >> +out: > >> + if (alen) { > > > > Just use more goto if needed. This is hard to follow when we come here > > and when not. > > > >> + err = 0; > >> + *new_len = alen; > >> + *new_lcn = alcn; > >> > >> - if (opt & ALLOCATE_MFT) > >> - goto out; > >> + ntfs_unmap_meta(sb, alcn, alen); > >> > >> - /* Set hint for next requests. */ > >> - sbi->used.next_free_lcn = *new_lcn + *new_len; > >> + /* Set hint for next requests. */ > >> + if (!(opt & ALLOCATE_MFT)) > >> + sbi->used.next_free_lcn = alcn + alen; > >> + } else { > >> + err = -ENOSPC; > >> + } > >> > >> -out: > > > > Example here > > > > err_up_write: > > > > and you can use that for many place > > > >> up_write(&wnd->rw_lock); > >> return err; > >> } > >> @@ -2226,7 +2209,7 @@ int ntfs_insert_security(struct ntfs_sb_info *sbi, > >> sii_e.sec_id = d_security->key.sec_id; > >> memcpy(&sii_e.sec_hdr, d_security, SIZEOF_SECURITY_HDR); > >> > >> - err = indx_insert_entry(indx_sii, ni, &sii_e.de, NULL, NULL); > >> + err = indx_insert_entry(indx_sii, ni, &sii_e.de, NULL, NULL, 0); > >> if (err) > >> goto out; > >> > >> @@ -2247,7 +2230,7 @@ int ntfs_insert_security(struct ntfs_sb_info *sbi, > >> > >> fnd_clear(fnd_sdh); > >> err = indx_insert_entry(indx_sdh, ni, &sdh_e.de, (void *)(size_t)1, > >> - fnd_sdh); > >> + fnd_sdh, 0); > >> if (err) > >> goto out; > >> > >> @@ -2385,7 +2368,7 @@ int ntfs_insert_reparse(struct ntfs_sb_info *sbi, __le32 rtag, > >> > >> mutex_lock_nested(&ni->ni_lock, NTFS_INODE_MUTEX_REPARSE); > >> > >> - err = indx_insert_entry(indx, ni, &re.de, NULL, NULL); > >> + err = indx_insert_entry(indx, ni, &re.de, NULL, NULL, 0); > >> > >> mark_inode_dirty(&ni->vfs_inode); > >> ni_unlock(ni); > >> diff --git a/fs/ntfs3/index.c b/fs/ntfs3/index.c > >> index 70ef59455b72..1224b8e42b3e 100644 > >> --- a/fs/ntfs3/index.c > >> +++ b/fs/ntfs3/index.c > >> @@ -1427,7 +1427,7 @@ static int indx_create_allocate(struct ntfs_index *indx, struct ntfs_inode *ni, > >> alloc->nres.valid_size = alloc->nres.data_size = cpu_to_le64(data_size); > >> > >> err = ni_insert_resident(ni, bitmap_size(1), ATTR_BITMAP, in->name, > >> - in->name_len, &bitmap, NULL); > >> + in->name_len, &bitmap, NULL, NULL); > >> if (err) > >> goto out2; > >> > >> @@ -1443,7 +1443,7 @@ static int indx_create_allocate(struct ntfs_index *indx, struct ntfs_inode *ni, > >> return 0; > >> > >> out2: > >> - mi_remove_attr(&ni->mi, alloc); > >> + mi_remove_attr(NULL, &ni->mi, alloc); > >> > >> out1: > >> run_deallocate(sbi, &run, false); > >> @@ -1529,24 +1529,24 @@ static int indx_add_allocate(struct ntfs_index *indx, struct ntfs_inode *ni, > >> /* > >> * indx_insert_into_root - Attempt to insert an entry into the index root. > >> * > >> + * @undo - True if we undoing previous remove. > >> * If necessary, it will twiddle the index b-tree. > >> */ > >> static int indx_insert_into_root(struct ntfs_index *indx, struct ntfs_inode *ni, > >> const struct NTFS_DE *new_de, > >> struct NTFS_DE *root_de, const void *ctx, > >> - struct ntfs_fnd *fnd) > >> + struct ntfs_fnd *fnd, bool undo) > >> { > >> int err = 0; > >> struct NTFS_DE *e, *e0, *re; > >> struct mft_inode *mi; > >> struct ATTRIB *attr; > >> - struct MFT_REC *rec; > >> struct INDEX_HDR *hdr; > >> struct indx_node *n; > >> CLST new_vbn; > >> __le64 *sub_vbn, t_vbn; > >> u16 new_de_size; > >> - u32 hdr_used, hdr_total, asize, used, to_move; > >> + u32 hdr_used, hdr_total, asize, to_move; > >> u32 root_size, new_root_size; > >> struct ntfs_sb_info *sbi; > >> int ds_root; > >> @@ -1559,12 +1559,11 @@ static int indx_insert_into_root(struct ntfs_index *indx, struct ntfs_inode *ni, > >> > >> /* > >> * Try easy case: > >> - * hdr_insert_de will succeed if there's room the root for the new entry. > >> + * hdr_insert_de will succeed if there's > >> + * room the root for the new entry. > > > > Not relevant for this patch. > > > >> */ > >> hdr = &root->ihdr; > >> sbi = ni->mi.sbi; > >> - rec = mi->mrec; > >> - used = le32_to_cpu(rec->used); > >> new_de_size = le16_to_cpu(new_de->size); > >> hdr_used = le32_to_cpu(hdr->used); > >> hdr_total = le32_to_cpu(hdr->total); > >> @@ -1573,9 +1572,9 @@ static int indx_insert_into_root(struct ntfs_index *indx, struct ntfs_inode *ni, > >> > >> ds_root = new_de_size + hdr_used - hdr_total; > >> > >> - if (used + ds_root < sbi->max_bytes_per_attr) { > >> - /* Make a room for new elements. */ > >> - mi_resize_attr(mi, attr, ds_root); > >> + /* If 'undo' is set then reduce requirements. */ > >> + if ((undo || asize + ds_root < sbi->max_bytes_per_attr) && > >> + mi_resize_attr(mi, attr, ds_root)) { > >> hdr->total = cpu_to_le32(hdr_total + ds_root); > >> e = hdr_insert_de(indx, hdr, new_de, root_de, ctx); > >> WARN_ON(!e); > >> @@ -1629,7 +1628,7 @@ static int indx_insert_into_root(struct ntfs_index *indx, struct ntfs_inode *ni, > >> sizeof(u64); > >> ds_root = new_root_size - root_size; > >> > >> - if (ds_root > 0 && used + ds_root > sbi->max_bytes_per_attr) { > >> + if (ds_root > 0 && asize + ds_root > sbi->max_bytes_per_attr) { > >> /* Make root external. */ > >> err = -EOPNOTSUPP; > >> goto out_free_re; > >> @@ -1710,7 +1709,7 @@ static int indx_insert_into_root(struct ntfs_index *indx, struct ntfs_inode *ni, > >> > >> put_indx_node(n); > >> fnd_clear(fnd); > >> - err = indx_insert_entry(indx, ni, new_de, ctx, fnd); > >> + err = indx_insert_entry(indx, ni, new_de, ctx, fnd, undo); > >> goto out_free_root; > >> } > >> > >> @@ -1854,7 +1853,7 @@ indx_insert_into_buffer(struct ntfs_index *indx, struct ntfs_inode *ni, > >> */ > >> if (!level) { > >> /* Insert in root. */ > >> - err = indx_insert_into_root(indx, ni, up_e, NULL, ctx, fnd); > >> + err = indx_insert_into_root(indx, ni, up_e, NULL, ctx, fnd, 0); > >> if (err) > >> goto out; > >> } else { > >> @@ -1876,10 +1875,12 @@ indx_insert_into_buffer(struct ntfs_index *indx, struct ntfs_inode *ni, > >> > >> /* > >> * indx_insert_entry - Insert new entry into index. > >> + * > >> + * @undo - True if we undoing previous remove. > >> */ > >> int indx_insert_entry(struct ntfs_index *indx, struct ntfs_inode *ni, > >> const struct NTFS_DE *new_de, const void *ctx, > >> - struct ntfs_fnd *fnd) > >> + struct ntfs_fnd *fnd, bool undo) > >> { > >> int err; > >> int diff; > >> @@ -1925,7 +1926,7 @@ int indx_insert_entry(struct ntfs_index *indx, struct ntfs_inode *ni, > >> * new entry into it. > >> */ > >> err = indx_insert_into_root(indx, ni, new_de, fnd->root_de, ctx, > >> - fnd); > >> + fnd, undo); > >> if (err) > >> goto out; > >> } else { > >> @@ -2302,7 +2303,7 @@ int indx_delete_entry(struct ntfs_index *indx, struct ntfs_inode *ni, > >> fnd->level - 1, > >> fnd) > >> : indx_insert_into_root(indx, ni, re, e, > >> - ctx, fnd); > >> + ctx, fnd, 0); > >> kfree(re); > >> > >> if (err) > >> @@ -2507,7 +2508,7 @@ int indx_delete_entry(struct ntfs_index *indx, struct ntfs_inode *ni, > >> * Re-insert the entry into the tree. > >> * Find the spot the tree where we want to insert the new entry. > >> */ > >> - err = indx_insert_entry(indx, ni, me, ctx, fnd); > >> + err = indx_insert_entry(indx, ni, me, ctx, fnd, 0); > >> kfree(me); > >> if (err) > >> goto out; > >> @@ -2595,10 +2596,8 @@ int indx_update_dup(struct ntfs_inode *ni, struct ntfs_sb_info *sbi, > >> struct ntfs_index *indx = &ni->dir; > >> > >> fnd = fnd_get(); > >> - if (!fnd) { > >> - err = -ENOMEM; > >> - goto out1; > >> - } > >> + if (!fnd) > >> + return -ENOMEM; > >> > >> root = indx_get_root(indx, ni, NULL, &mi); > >> if (!root) { > >> @@ -2645,7 +2644,5 @@ int indx_update_dup(struct ntfs_inode *ni, struct ntfs_sb_info *sbi, > >> > >> out: > >> fnd_put(fnd); > >> - > >> -out1: > >> return err; > >> } > >> diff --git a/fs/ntfs3/inode.c b/fs/ntfs3/inode.c > >> index b86ec7dd731c..8f72066b3229 100644 > >> --- a/fs/ntfs3/inode.c > >> +++ b/fs/ntfs3/inode.c > >> @@ -399,6 +399,12 @@ static struct inode *ntfs_read_mft(struct inode *inode, > >> goto out; > >> } > >> > >> + if (names != le16_to_cpu(rec->hard_links)) { > >> + /* Correct minor error on the fly. Do not mark inode as dirty. */ > >> + rec->hard_links = cpu_to_le16(names); > >> + ni->mi.dirty = true; > >> + } > >> + > >> set_nlink(inode, names); > >> > >> if (S_ISDIR(mode)) { > >> @@ -1279,6 +1285,7 @@ struct inode *ntfs_create_inode(struct user_namespace *mnt_userns, > >> } > >> inode = &ni->vfs_inode; > >> inode_init_owner(mnt_userns, inode, dir, mode); > >> + mode = inode->i_mode; > >> > >> inode->i_atime = inode->i_mtime = inode->i_ctime = ni->i_crtime = > >> current_time(inode); > >> @@ -1371,6 +1378,7 @@ struct inode *ntfs_create_inode(struct user_namespace *mnt_userns, > >> attr = Add2Ptr(attr, asize); > >> } > >> > >> + attr->id = cpu_to_le16(aid++); > >> if (fa & FILE_ATTRIBUTE_DIRECTORY) { > >> /* > >> * Regular directory or symlink to directory. > >> @@ -1381,7 +1389,6 @@ struct inode *ntfs_create_inode(struct user_namespace *mnt_userns, > >> > >> attr->type = ATTR_ROOT; > >> attr->size = cpu_to_le32(asize); > >> - attr->id = cpu_to_le16(aid++); > >> > >> attr->name_len = ARRAY_SIZE(I30_NAME); > >> attr->name_off = SIZEOF_RESIDENT_LE; > >> @@ -1412,52 +1419,46 @@ struct inode *ntfs_create_inode(struct user_namespace *mnt_userns, > >> /* Insert empty ATTR_DATA */ > >> attr->type = ATTR_DATA; > >> attr->size = cpu_to_le32(SIZEOF_RESIDENT); > >> - attr->id = cpu_to_le16(aid++); > >> attr->name_off = SIZEOF_RESIDENT_LE; > >> attr->res.data_off = SIZEOF_RESIDENT_LE; > >> - } else { > >> + } else if (S_ISREG(mode)) { > > > > This should definetly be own patch. Way too much noice. And yes I think > > this is a good change but if this is in this patch then how do we see > > what is relevant? > > > >> /* > >> - * Regular file or node. > >> + * Regular file. Create empty non resident data attribute. > >> */ > >> attr->type = ATTR_DATA; > >> - attr->id = cpu_to_le16(aid++); > >> - > >> - if (S_ISREG(mode)) { > >> - /* Create empty non resident data attribute. */ > >> - attr->non_res = 1; > >> - attr->nres.evcn = cpu_to_le64(-1ll); > >> - if (fa & FILE_ATTRIBUTE_SPARSE_FILE) { > >> - attr->size = > >> - cpu_to_le32(SIZEOF_NONRESIDENT_EX + 8); > >> - attr->name_off = SIZEOF_NONRESIDENT_EX_LE; > >> - attr->flags = ATTR_FLAG_SPARSED; > >> - asize = SIZEOF_NONRESIDENT_EX + 8; > >> - } else if (fa & FILE_ATTRIBUTE_COMPRESSED) { > >> - attr->size = > >> - cpu_to_le32(SIZEOF_NONRESIDENT_EX + 8); > >> - attr->name_off = SIZEOF_NONRESIDENT_EX_LE; > >> - attr->flags = ATTR_FLAG_COMPRESSED; > >> - attr->nres.c_unit = COMPRESSION_UNIT; > >> - asize = SIZEOF_NONRESIDENT_EX + 8; > >> - } else { > >> - attr->size = > >> - cpu_to_le32(SIZEOF_NONRESIDENT + 8); > >> - attr->name_off = SIZEOF_NONRESIDENT_LE; > >> - asize = SIZEOF_NONRESIDENT + 8; > >> - } > >> - attr->nres.run_off = attr->name_off; > >> + attr->non_res = 1; > >> + attr->nres.evcn = cpu_to_le64(-1ll); > >> + if (fa & FILE_ATTRIBUTE_SPARSE_FILE) { > >> + attr->size = cpu_to_le32(SIZEOF_NONRESIDENT_EX + 8); > >> + attr->name_off = SIZEOF_NONRESIDENT_EX_LE; > >> + attr->flags = ATTR_FLAG_SPARSED; > >> + asize = SIZEOF_NONRESIDENT_EX + 8; > >> + } else if (fa & FILE_ATTRIBUTE_COMPRESSED) { > >> + attr->size = cpu_to_le32(SIZEOF_NONRESIDENT_EX + 8); > >> + attr->name_off = SIZEOF_NONRESIDENT_EX_LE; > >> + attr->flags = ATTR_FLAG_COMPRESSED; > >> + attr->nres.c_unit = COMPRESSION_UNIT; > >> + asize = SIZEOF_NONRESIDENT_EX + 8; > >> } else { > >> - /* Create empty resident data attribute. */ > >> - attr->size = cpu_to_le32(SIZEOF_RESIDENT); > >> - attr->name_off = SIZEOF_RESIDENT_LE; > >> - if (fa & FILE_ATTRIBUTE_SPARSE_FILE) > >> - attr->flags = ATTR_FLAG_SPARSED; > >> - else if (fa & FILE_ATTRIBUTE_COMPRESSED) > >> - attr->flags = ATTR_FLAG_COMPRESSED; > >> - attr->res.data_off = SIZEOF_RESIDENT_LE; > >> - asize = SIZEOF_RESIDENT; > >> - ni->ni_flags |= NI_FLAG_RESIDENT; > >> + attr->size = cpu_to_le32(SIZEOF_NONRESIDENT + 8); > >> + attr->name_off = SIZEOF_NONRESIDENT_LE; > >> + asize = SIZEOF_NONRESIDENT + 8; > >> } > >> + attr->nres.run_off = attr->name_off; > >> + } else { > >> + /* > >> + * Node. Create empty resident data attribute. > >> + */ > >> + attr->type = ATTR_DATA; > >> + attr->size = cpu_to_le32(SIZEOF_RESIDENT); > >> + attr->name_off = SIZEOF_RESIDENT_LE; > >> + if (fa & FILE_ATTRIBUTE_SPARSE_FILE) > >> + attr->flags = ATTR_FLAG_SPARSED; > >> + else if (fa & FILE_ATTRIBUTE_COMPRESSED) > >> + attr->flags = ATTR_FLAG_COMPRESSED; > >> + attr->res.data_off = SIZEOF_RESIDENT_LE; > >> + asize = SIZEOF_RESIDENT; > >> + ni->ni_flags |= NI_FLAG_RESIDENT; > >> } > >> > >> if (S_ISDIR(mode)) { > >> @@ -1485,7 +1486,8 @@ struct inode *ntfs_create_inode(struct user_namespace *mnt_userns, > >> asize = ALIGN(SIZEOF_RESIDENT + nsize, 8); > >> t16 = PtrOffset(rec, attr); > >> > >> - if (asize + t16 + 8 > sbi->record_size) { > >> + /* 0x78 - the size of EA + EAINFO to store WSL */ > >> + if (asize + t16 + 0x78 + 8 > sbi->record_size) { > >> CLST alen; > >> CLST clst = bytes_to_cluster(sbi, nsize); > >> > >> @@ -1545,20 +1547,15 @@ struct inode *ntfs_create_inode(struct user_namespace *mnt_userns, > >> rec->next_attr_id = cpu_to_le16(aid); > >> > >> /* Step 2: Add new name in index. */ > >> - err = indx_insert_entry(&dir_ni->dir, dir_ni, new_de, sbi, fnd); > >> + err = indx_insert_entry(&dir_ni->dir, dir_ni, new_de, sbi, fnd, 0); > >> if (err) > >> goto out6; > >> > >> - /* Update current directory record. */ > >> - mark_inode_dirty(dir); > >> - > > > > This was also probably just fix it is own? > > > >> inode->i_generation = le16_to_cpu(rec->seq); > >> > >> dir->i_mtime = dir->i_ctime = inode->i_atime; > >> > >> if (S_ISDIR(mode)) { > >> - if (dir->i_mode & S_ISGID) > >> - mode |= S_ISGID; > > > > This also is not relevant for this patch. Yes dead code but not releted > > to this one. This kind of stuff takes lot of reviewers time. > > > > > > I do not even feel like i will continue this review. There is just way > > too much stuff and would definetly nack. But as this already is in ntfs3 > > master I just don't know what to do. I cc fsdevel so maybe they can help > > a little bit. > > > > Argillander > > > >> inode->i_op = &ntfs_dir_inode_operations; > >> inode->i_fop = &ntfs_dir_operations; > >> } else if (S_ISLNK(mode)) { > >> @@ -1601,8 +1598,8 @@ struct inode *ntfs_create_inode(struct user_namespace *mnt_userns, > >> d_instantiate(dentry, inode); > >> > >> ntfs_save_wsl_perm(inode); > >> - mark_inode_dirty(inode); > >> mark_inode_dirty(dir); > >> + mark_inode_dirty(inode); > >> > >> /* Normal exit. */ > >> goto out2; > >> @@ -1646,61 +1643,36 @@ struct inode *ntfs_create_inode(struct user_namespace *mnt_userns, > >> int ntfs_link_inode(struct inode *inode, struct dentry *dentry) > >> { > >> int err; > >> - struct inode *dir = d_inode(dentry->d_parent); > >> - struct ntfs_inode *dir_ni = ntfs_i(dir); > >> struct ntfs_inode *ni = ntfs_i(inode); > >> - struct super_block *sb = inode->i_sb; > >> - struct ntfs_sb_info *sbi = sb->s_fs_info; > >> - const struct qstr *name = &dentry->d_name; > >> - struct NTFS_DE *new_de = NULL; > >> - struct ATTR_FILE_NAME *fname; > >> - struct ATTRIB *attr; > >> - u16 key_size; > >> - struct INDEX_ROOT *dir_root; > >> - > >> - dir_root = indx_get_root(&dir_ni->dir, dir_ni, NULL, NULL); > >> - if (!dir_root) > >> - return -EINVAL; > >> + struct ntfs_sb_info *sbi = inode->i_sb->s_fs_info; > >> + struct NTFS_DE *de; > >> + struct ATTR_FILE_NAME *de_name; > >> > >> /* Allocate PATH_MAX bytes. */ > >> - new_de = __getname(); > >> - if (!new_de) > >> + de = __getname(); > >> + if (!de) > >> return -ENOMEM; > >> > >> - /* Mark rw ntfs as dirty. It will be cleared at umount. */ > >> - ntfs_set_state(ni->mi.sbi, NTFS_DIRTY_DIRTY); > >> - > >> - /* Insert file name. */ > >> - err = fill_name_de(sbi, new_de, name, NULL); > >> - if (err) > >> - goto out; > >> - > >> - key_size = le16_to_cpu(new_de->key_size); > >> - err = ni_insert_resident(ni, key_size, ATTR_NAME, NULL, 0, &attr, NULL); > >> - if (err) > >> - goto out; > >> - > >> - mi_get_ref(&ni->mi, &new_de->ref); > >> - > >> - fname = (struct ATTR_FILE_NAME *)(new_de + 1); > >> - mi_get_ref(&dir_ni->mi, &fname->home); > >> - fname->dup.cr_time = fname->dup.m_time = fname->dup.c_time = > >> - fname->dup.a_time = kernel2nt(&inode->i_ctime); > >> - fname->dup.alloc_size = fname->dup.data_size = 0; > >> - fname->dup.fa = ni->std_fa; > >> - fname->dup.ea_size = fname->dup.reparse = 0; > >> - > >> - memcpy(Add2Ptr(attr, SIZEOF_RESIDENT), fname, key_size); > >> + /* Mark rw ntfs as dirty. It will be cleared at umount. */ > >> + ntfs_set_state(sbi, NTFS_DIRTY_DIRTY); > >> > >> - err = indx_insert_entry(&dir_ni->dir, dir_ni, new_de, sbi, NULL); > >> + /* Construct 'de'. */ > >> + err = fill_name_de(sbi, de, &dentry->d_name, NULL); > >> if (err) > >> goto out; > >> > >> - le16_add_cpu(&ni->mi.mrec->hard_links, 1); > >> - ni->mi.dirty = true; > >> + de_name = (struct ATTR_FILE_NAME *)(de + 1); > >> + /* Fill duplicate info. */ > >> + de_name->dup.cr_time = de_name->dup.m_time = de_name->dup.c_time = > >> + de_name->dup.a_time = kernel2nt(&inode->i_ctime); > >> + de_name->dup.alloc_size = de_name->dup.data_size = > >> + cpu_to_le64(inode->i_size); > >> + de_name->dup.fa = ni->std_fa; > >> + de_name->dup.ea_size = de_name->dup.reparse = 0; > >> > >> + err = ni_add_name(ntfs_i(d_inode(dentry->d_parent)), ni, de); > >> out: > >> - __putname(new_de); > >> + __putname(de); > >> return err; > >> } > >> > >> @@ -1713,113 +1685,56 @@ int ntfs_link_inode(struct inode *inode, struct dentry *dentry) > >> int ntfs_unlink_inode(struct inode *dir, const struct dentry *dentry) > >> { > >> int err; > >> - struct super_block *sb = dir->i_sb; > >> - struct ntfs_sb_info *sbi = sb->s_fs_info; > >> + struct ntfs_sb_info *sbi = dir->i_sb->s_fs_info; > >> struct inode *inode = d_inode(dentry); > >> struct ntfs_inode *ni = ntfs_i(inode); > >> - const struct qstr *name = &dentry->d_name; > >> struct ntfs_inode *dir_ni = ntfs_i(dir); > >> - struct ntfs_index *indx = &dir_ni->dir; > >> - struct cpu_str *uni = NULL; > >> - struct ATTR_FILE_NAME *fname; > >> - u8 name_type; > >> - struct ATTR_LIST_ENTRY *le; > >> - struct MFT_REF ref; > >> - bool is_dir = S_ISDIR(inode->i_mode); > >> - struct INDEX_ROOT *dir_root; > >> + struct NTFS_DE *de, *de2 = NULL; > >> + int undo_remove; > >> > >> - dir_root = indx_get_root(indx, dir_ni, NULL, NULL); > >> - if (!dir_root) > >> + if (ntfs_is_meta_file(sbi, ni->mi.rno)) > >> return -EINVAL; > >> > >> + /* Allocate PATH_MAX bytes. */ > >> + de = __getname(); > >> + if (!de) > >> + return -ENOMEM; > >> + > >> ni_lock(ni); > >> > >> - if (is_dir && !dir_is_empty(inode)) { > >> + if (S_ISDIR(inode->i_mode) && !dir_is_empty(inode)) { > >> err = -ENOTEMPTY; > >> - goto out1; > >> - } > >> - > >> - if (ntfs_is_meta_file(sbi, inode->i_ino)) { > >> - err = -EINVAL; > >> - goto out1; > >> - } > >> - > >> - /* Allocate PATH_MAX bytes. */ > >> - uni = __getname(); > >> - if (!uni) { > >> - err = -ENOMEM; > >> - goto out1; > >> + goto out; > >> } > >> > >> - /* Convert input string to unicode. */ > >> - err = ntfs_nls_to_utf16(sbi, name->name, name->len, uni, NTFS_NAME_LEN, > >> - UTF16_HOST_ENDIAN); > >> + err = fill_name_de(sbi, de, &dentry->d_name, NULL); > >> if (err < 0) > >> - goto out2; > >> - > >> - /* Mark rw ntfs as dirty. It will be cleared at umount. */ > >> - ntfs_set_state(sbi, NTFS_DIRTY_DIRTY); > >> - > >> - /* Find name in record. */ > >> - mi_get_ref(&dir_ni->mi, &ref); > >> - > >> - le = NULL; > >> - fname = ni_fname_name(ni, uni, &ref, &le); > >> - if (!fname) { > >> - err = -ENOENT; > >> - goto out3; > >> - } > >> - > >> - name_type = paired_name(fname->type); > >> - > >> - err = indx_delete_entry(indx, dir_ni, fname, fname_full_size(fname), > >> - sbi); > >> - if (err) > >> - goto out3; > >> - > >> - /* Then remove name from MFT. */ > >> - ni_remove_attr_le(ni, attr_from_name(fname), le); > >> - > >> - le16_add_cpu(&ni->mi.mrec->hard_links, -1); > >> - ni->mi.dirty = true; > >> - > >> - if (name_type != FILE_NAME_POSIX) { > >> - /* Now we should delete name by type. */ > >> - fname = ni_fname_type(ni, name_type, &le); > >> - if (fname) { > >> - err = indx_delete_entry(indx, dir_ni, fname, > >> - fname_full_size(fname), sbi); > >> - if (err) > >> - goto out3; > >> + goto out; > >> > >> - ni_remove_attr_le(ni, attr_from_name(fname), le); > >> + undo_remove = 0; > >> + err = ni_remove_name(dir_ni, ni, de, &de2, &undo_remove); > >> > >> - le16_add_cpu(&ni->mi.mrec->hard_links, -1); > >> - } > >> - } > >> -out3: > >> - switch (err) { > >> - case 0: > >> + if (!err) { > >> drop_nlink(inode); > >> - break; > >> - case -ENOTEMPTY: > >> - case -ENOSPC: > >> - case -EROFS: > >> - break; > >> - default: > >> + dir->i_mtime = dir->i_ctime = current_time(dir); > >> + mark_inode_dirty(dir); > >> + inode->i_ctime = dir->i_ctime; > >> + if (inode->i_nlink) > >> + mark_inode_dirty(inode); > >> + } else if (!ni_remove_name_undo(dir_ni, ni, de, de2, undo_remove)) { > >> make_bad_inode(inode); > >> + ntfs_inode_err(inode, "failed to undo unlink"); > >> + ntfs_set_state(sbi, NTFS_DIRTY_ERROR); > >> + } else { > >> + if (ni_is_dirty(dir)) > >> + mark_inode_dirty(dir); > >> + if (ni_is_dirty(inode)) > >> + mark_inode_dirty(inode); > >> } > >> > >> - dir->i_mtime = dir->i_ctime = current_time(dir); > >> - mark_inode_dirty(dir); > >> - inode->i_ctime = dir->i_ctime; > >> - if (inode->i_nlink) > >> - mark_inode_dirty(inode); > >> - > >> -out2: > >> - __putname(uni); > >> -out1: > >> +out: > >> ni_unlock(ni); > >> + __putname(de); > >> return err; > >> } > >> > >> diff --git a/fs/ntfs3/namei.c b/fs/ntfs3/namei.c > >> index f79a399bd015..e58415d07132 100644 > >> --- a/fs/ntfs3/namei.c > >> +++ b/fs/ntfs3/namei.c > >> @@ -152,12 +152,14 @@ static int ntfs_link(struct dentry *ode, struct inode *dir, struct dentry *de) > >> if (inode != dir) > >> ni_lock(ni); > >> > >> - dir->i_ctime = dir->i_mtime = inode->i_ctime = current_time(inode); > >> inc_nlink(inode); > >> ihold(inode); > >> > >> err = ntfs_link_inode(inode, de); > >> + > >> if (!err) { > >> + dir->i_ctime = dir->i_mtime = inode->i_ctime = > >> + current_time(dir); > >> mark_inode_dirty(inode); > >> mark_inode_dirty(dir); > >> d_instantiate(de, inode); > >> @@ -249,25 +251,26 @@ static int ntfs_rmdir(struct inode *dir, struct dentry *dentry) > >> /* > >> * ntfs_rename - inode_operations::rename > >> */ > >> -static int ntfs_rename(struct user_namespace *mnt_userns, struct inode *old_dir, > >> - struct dentry *old_dentry, struct inode *new_dir, > >> +static int ntfs_rename(struct user_namespace *mnt_userns, struct inode *dir, > >> + struct dentry *dentry, struct inode *new_dir, > >> struct dentry *new_dentry, u32 flags) > >> { > >> int err; > >> - struct super_block *sb = old_dir->i_sb; > >> + struct super_block *sb = dir->i_sb; > >> struct ntfs_sb_info *sbi = sb->s_fs_info; > >> - struct ntfs_inode *old_dir_ni = ntfs_i(old_dir); > >> + struct ntfs_inode *dir_ni = ntfs_i(dir); > >> struct ntfs_inode *new_dir_ni = ntfs_i(new_dir); > >> - struct ntfs_inode *old_ni; > >> - struct ATTR_FILE_NAME *old_name, *new_name, *fname; > >> - u8 name_type; > >> - bool is_same; > >> - struct inode *old_inode, *new_inode; > >> - struct NTFS_DE *old_de, *new_de; > >> - struct ATTRIB *attr; > >> - struct ATTR_LIST_ENTRY *le; > >> - u16 new_de_key_size; > >> - > >> + struct inode *inode = d_inode(dentry); > >> + struct ntfs_inode *ni = ntfs_i(inode); > >> + struct inode *new_inode = d_inode(new_dentry); > >> + struct NTFS_DE *de, *new_de; > >> + bool is_same, is_bad; > >> + /* > >> + * de - memory of PATH_MAX bytes: > >> + * [0-1024) - original name (dentry->d_name) > >> + * [1024-2048) - paired to original name, usually DOS variant of dentry->d_name > >> + * [2048-3072) - new name (new_dentry->d_name) > >> + */ > >> static_assert(SIZEOF_ATTRIBUTE_FILENAME_MAX + SIZEOF_RESIDENT < 1024); > >> static_assert(SIZEOF_ATTRIBUTE_FILENAME_MAX + sizeof(struct NTFS_DE) < > >> 1024); > >> @@ -276,24 +279,18 @@ static int ntfs_rename(struct user_namespace *mnt_userns, struct inode *old_dir, > >> if (flags & ~RENAME_NOREPLACE) > >> return -EINVAL; > >> > >> - old_inode = d_inode(old_dentry); > >> - new_inode = d_inode(new_dentry); > >> - > >> - old_ni = ntfs_i(old_inode); > >> + is_same = dentry->d_name.len == new_dentry->d_name.len && > >> + !memcmp(dentry->d_name.name, new_dentry->d_name.name, > >> + dentry->d_name.len); > >> > >> - is_same = old_dentry->d_name.len == new_dentry->d_name.len && > >> - !memcmp(old_dentry->d_name.name, new_dentry->d_name.name, > >> - old_dentry->d_name.len); > >> - > >> - if (is_same && old_dir == new_dir) { > >> + if (is_same && dir == new_dir) { > >> /* Nothing to do. */ > >> - err = 0; > >> - goto out; > >> + return 0; > >> } > >> > >> - if (ntfs_is_meta_file(sbi, old_inode->i_ino)) { > >> - err = -EINVAL; > >> - goto out; > >> + if (ntfs_is_meta_file(sbi, inode->i_ino)) { > >> + /* Should we print an error? */ > >> + return -EINVAL; > >> } > >> > >> if (new_inode) { > >> @@ -304,168 +301,61 @@ static int ntfs_rename(struct user_namespace *mnt_userns, struct inode *old_dir, > >> ni_unlock(new_dir_ni); > >> dput(new_dentry); > >> if (err) > >> - goto out; > >> + return err; > >> } > >> > >> /* Allocate PATH_MAX bytes. */ > >> - old_de = __getname(); > >> - if (!old_de) { > >> - err = -ENOMEM; > >> - goto out; > >> - } > >> + de = __getname(); > >> + if (!de) > >> + return -ENOMEM; > >> > >> - err = fill_name_de(sbi, old_de, &old_dentry->d_name, NULL); > >> + /* Translate dentry->d_name into unicode form. */ > >> + err = fill_name_de(sbi, de, &dentry->d_name, NULL); > >> if (err < 0) > >> - goto out1; > >> - > >> - old_name = (struct ATTR_FILE_NAME *)(old_de + 1); > >> + goto out; > >> > >> if (is_same) { > >> - new_de = old_de; > >> + /* Reuse 'de'. */ > >> + new_de = de; > >> } else { > >> - new_de = Add2Ptr(old_de, 1024); > >> + /* Translate new_dentry->d_name into unicode form. */ > >> + new_de = Add2Ptr(de, 2048); > >> err = fill_name_de(sbi, new_de, &new_dentry->d_name, NULL); > >> if (err < 0) > >> - goto out1; > >> - } > >> - > >> - ni_lock_dir(old_dir_ni); > >> - ni_lock(old_ni); > >> - > >> - mi_get_ref(&old_dir_ni->mi, &old_name->home); > >> - > >> - /* Get pointer to file_name in MFT. */ > >> - fname = ni_fname_name(old_ni, (struct cpu_str *)&old_name->name_len, > >> - &old_name->home, &le); > >> - if (!fname) { > >> - err = -EINVAL; > >> - goto out2; > >> + goto out; > >> } > >> > >> - /* Copy fname info from record into new fname. */ > >> - new_name = (struct ATTR_FILE_NAME *)(new_de + 1); > >> - memcpy(&new_name->dup, &fname->dup, sizeof(fname->dup)); > >> - > >> - name_type = paired_name(fname->type); > >> - > >> - /* Remove first name from directory. */ > >> - err = indx_delete_entry(&old_dir_ni->dir, old_dir_ni, old_de + 1, > >> - le16_to_cpu(old_de->key_size), sbi); > >> - if (err) > >> - goto out3; > >> - > >> - /* Remove first name from MFT. */ > >> - err = ni_remove_attr_le(old_ni, attr_from_name(fname), le); > >> - if (err) > >> - goto out4; > >> - > >> - le16_add_cpu(&old_ni->mi.mrec->hard_links, -1); > >> - old_ni->mi.dirty = true; > >> - > >> - if (name_type != FILE_NAME_POSIX) { > >> - /* Get paired name. */ > >> - fname = ni_fname_type(old_ni, name_type, &le); > >> - if (fname) { > >> - /* Remove second name from directory. */ > >> - err = indx_delete_entry(&old_dir_ni->dir, old_dir_ni, > >> - fname, fname_full_size(fname), > >> - sbi); > >> - if (err) > >> - goto out5; > >> - > >> - /* Remove second name from MFT. */ > >> - err = ni_remove_attr_le(old_ni, attr_from_name(fname), > >> - le); > >> - if (err) > >> - goto out6; > >> - > >> - le16_add_cpu(&old_ni->mi.mrec->hard_links, -1); > >> - old_ni->mi.dirty = true; > >> + ni_lock_dir(dir_ni); > >> + ni_lock(ni); > >> + > >> + is_bad = false; > >> + err = ni_rename(dir_ni, new_dir_ni, ni, de, new_de, &is_bad); > >> + if (is_bad) { > >> + /* Restore after failed rename failed too. */ > >> + make_bad_inode(inode); > >> + ntfs_inode_err(inode, "failed to undo rename"); > >> + ntfs_set_state(sbi, NTFS_DIRTY_ERROR); > >> + } else if (!err) { > >> + inode->i_ctime = dir->i_ctime = dir->i_mtime = > >> + current_time(dir); > >> + mark_inode_dirty(inode); > >> + mark_inode_dirty(dir); > >> + if (dir != new_dir) { > >> + new_dir->i_mtime = new_dir->i_ctime = dir->i_ctime; > >> + mark_inode_dirty(new_dir); > >> } > >> - } > >> - > >> - /* Add new name. */ > >> - mi_get_ref(&old_ni->mi, &new_de->ref); > >> - mi_get_ref(&ntfs_i(new_dir)->mi, &new_name->home); > >> - > >> - new_de_key_size = le16_to_cpu(new_de->key_size); > >> - > >> - /* Insert new name in MFT. */ > >> - err = ni_insert_resident(old_ni, new_de_key_size, ATTR_NAME, NULL, 0, > >> - &attr, NULL); > >> - if (err) > >> - goto out7; > >> - > >> - attr->res.flags = RESIDENT_FLAG_INDEXED; > >> - > >> - memcpy(Add2Ptr(attr, SIZEOF_RESIDENT), new_name, new_de_key_size); > >> - > >> - le16_add_cpu(&old_ni->mi.mrec->hard_links, 1); > >> - old_ni->mi.dirty = true; > >> - > >> - /* Insert new name in directory. */ > >> - err = indx_insert_entry(&new_dir_ni->dir, new_dir_ni, new_de, sbi, > >> - NULL); > >> - if (err) > >> - goto out8; > >> > >> - if (IS_DIRSYNC(new_dir)) > >> - err = ntfs_sync_inode(old_inode); > >> - else > >> - mark_inode_dirty(old_inode); > >> + if (IS_DIRSYNC(dir)) > >> + ntfs_sync_inode(dir); > >> > >> - old_dir->i_ctime = old_dir->i_mtime = current_time(old_dir); > >> - if (IS_DIRSYNC(old_dir)) > >> - (void)ntfs_sync_inode(old_dir); > >> - else > >> - mark_inode_dirty(old_dir); > >> - > >> - if (old_dir != new_dir) { > >> - new_dir->i_mtime = new_dir->i_ctime = old_dir->i_ctime; > >> - mark_inode_dirty(new_dir); > >> - } > >> - > >> - if (old_inode) { > >> - old_inode->i_ctime = old_dir->i_ctime; > >> - mark_inode_dirty(old_inode); > >> + if (IS_DIRSYNC(new_dir)) > >> + ntfs_sync_inode(inode); > >> } > >> > >> - err = 0; > >> - /* Normal way* */ > >> - goto out2; > >> - > >> -out8: > >> - /* undo > >> - * ni_insert_resident(old_ni, new_de_key_size, ATTR_NAME, NULL, 0, > >> - * &attr, NULL); > >> - */ > >> - mi_remove_attr(&old_ni->mi, attr); > >> -out7: > >> - /* undo > >> - * ni_remove_attr_le(old_ni, attr_from_name(fname), le); > >> - */ > >> -out6: > >> - /* undo > >> - * indx_delete_entry(&old_dir_ni->dir, old_dir_ni, > >> - * fname, fname_full_size(fname), > >> - * sbi); > >> - */ > >> -out5: > >> - /* undo > >> - * ni_remove_attr_le(old_ni, attr_from_name(fname), le); > >> - */ > >> -out4: > >> - /* undo: > >> - * indx_delete_entry(&old_dir_ni->dir, old_dir_ni, old_de + 1, > >> - * old_de->key_size, NULL); > >> - */ > >> -out3: > >> -out2: > >> - ni_unlock(old_ni); > >> - ni_unlock(old_dir_ni); > >> -out1: > >> - __putname(old_de); > >> + ni_unlock(ni); > >> + ni_unlock(dir_ni); > >> out: > >> + __putname(de); > >> return err; > >> } > >> > >> diff --git a/fs/ntfs3/ntfs_fs.h b/fs/ntfs3/ntfs_fs.h > >> index 64ef92e16363..f9436cbbc347 100644 > >> --- a/fs/ntfs3/ntfs_fs.h > >> +++ b/fs/ntfs3/ntfs_fs.h > >> @@ -478,7 +478,7 @@ struct ATTR_STD_INFO *ni_std(struct ntfs_inode *ni); > >> struct ATTR_STD_INFO5 *ni_std5(struct ntfs_inode *ni); > >> void ni_clear(struct ntfs_inode *ni); > >> int ni_load_mi_ex(struct ntfs_inode *ni, CLST rno, struct mft_inode **mi); > >> -int ni_load_mi(struct ntfs_inode *ni, struct ATTR_LIST_ENTRY *le, > >> +int ni_load_mi(struct ntfs_inode *ni, const struct ATTR_LIST_ENTRY *le, > >> struct mft_inode **mi); > >> struct ATTRIB *ni_find_attr(struct ntfs_inode *ni, struct ATTRIB *attr, > >> struct ATTR_LIST_ENTRY **entry_o, > >> @@ -505,15 +505,18 @@ int ni_insert_nonresident(struct ntfs_inode *ni, enum ATTR_TYPE type, > >> struct mft_inode **mi); > >> int ni_insert_resident(struct ntfs_inode *ni, u32 data_size, > >> enum ATTR_TYPE type, const __le16 *name, u8 name_len, > >> - struct ATTRIB **new_attr, struct mft_inode **mi); > >> -int ni_remove_attr_le(struct ntfs_inode *ni, struct ATTRIB *attr, > >> - struct ATTR_LIST_ENTRY *le); > >> + struct ATTRIB **new_attr, struct mft_inode **mi, > >> + struct ATTR_LIST_ENTRY **le); > >> +void ni_remove_attr_le(struct ntfs_inode *ni, struct ATTRIB *attr, > >> + struct mft_inode *mi, struct ATTR_LIST_ENTRY *le); > >> int ni_delete_all(struct ntfs_inode *ni); > >> struct ATTR_FILE_NAME *ni_fname_name(struct ntfs_inode *ni, > >> const struct cpu_str *uni, > >> const struct MFT_REF *home, > >> + struct mft_inode **mi, > >> struct ATTR_LIST_ENTRY **entry); > >> struct ATTR_FILE_NAME *ni_fname_type(struct ntfs_inode *ni, u8 name_type, > >> + struct mft_inode **mi, > >> struct ATTR_LIST_ENTRY **entry); > >> int ni_new_attr_flags(struct ntfs_inode *ni, enum FILE_ATTRIBUTE new_fa); > >> enum REPARSE_SIGN ni_parse_reparse(struct ntfs_inode *ni, struct ATTRIB *attr, > >> @@ -528,6 +531,21 @@ int ni_read_frame(struct ntfs_inode *ni, u64 frame_vbo, struct page **pages, > >> u32 pages_per_frame); > >> int ni_write_frame(struct ntfs_inode *ni, struct page **pages, > >> u32 pages_per_frame); > >> +int ni_remove_name(struct ntfs_inode *dir_ni, struct ntfs_inode *ni, > >> + struct NTFS_DE *de, struct NTFS_DE **de2, int *undo_step); > >> + > >> +bool ni_remove_name_undo(struct ntfs_inode *dir_ni, struct ntfs_inode *ni, > >> + struct NTFS_DE *de, struct NTFS_DE *de2, > >> + int undo_step); > >> + > >> +int ni_add_name(struct ntfs_inode *dir_ni, struct ntfs_inode *ni, > >> + struct NTFS_DE *de); > >> + > >> +int ni_rename(struct ntfs_inode *dir_ni, struct ntfs_inode *new_dir_ni, > >> + struct ntfs_inode *ni, struct NTFS_DE *de, struct NTFS_DE *new_de, > >> + bool *is_bad); > >> + > >> +bool ni_is_dirty(struct inode *inode); > >> > >> /* Globals from fslog.c */ > >> int log_replay(struct ntfs_inode *ni, bool *initialized); > >> @@ -631,7 +649,7 @@ int indx_find_raw(struct ntfs_index *indx, struct ntfs_inode *ni, > >> size_t *off, struct ntfs_fnd *fnd); > >> int indx_insert_entry(struct ntfs_index *indx, struct ntfs_inode *ni, > >> const struct NTFS_DE *new_de, const void *param, > >> - struct ntfs_fnd *fnd); > >> + struct ntfs_fnd *fnd, bool undo); > >> int indx_delete_entry(struct ntfs_index *indx, struct ntfs_inode *ni, > >> const void *key, u32 key_len, const void *param); > >> int indx_update_dup(struct ntfs_inode *ni, struct ntfs_sb_info *sbi, > >> @@ -694,7 +712,8 @@ struct ATTRIB *mi_insert_attr(struct mft_inode *mi, enum ATTR_TYPE type, > >> const __le16 *name, u8 name_len, u32 asize, > >> u16 name_off); > >> > >> -bool mi_remove_attr(struct mft_inode *mi, struct ATTRIB *attr); > >> +bool mi_remove_attr(struct ntfs_inode *ni, struct mft_inode *mi, > >> + struct ATTRIB *attr); > >> bool mi_resize_attr(struct mft_inode *mi, struct ATTRIB *attr, int bytes); > >> int mi_pack_runs(struct mft_inode *mi, struct ATTRIB *attr, > >> struct runs_tree *run, CLST len); > >> diff --git a/fs/ntfs3/record.c b/fs/ntfs3/record.c > >> index d48a5e6c5045..61e3f2fb619f 100644 > >> --- a/fs/ntfs3/record.c > >> +++ b/fs/ntfs3/record.c > >> @@ -489,7 +489,8 @@ struct ATTRIB *mi_insert_attr(struct mft_inode *mi, enum ATTR_TYPE type, > >> * > >> * NOTE: The source attr will point to next attribute. > >> */ > >> -bool mi_remove_attr(struct mft_inode *mi, struct ATTRIB *attr) > >> +bool mi_remove_attr(struct ntfs_inode *ni, struct mft_inode *mi, > >> + struct ATTRIB *attr) > >> { > >> struct MFT_REC *rec = mi->mrec; > >> u32 aoff = PtrOffset(rec, attr); > >> @@ -499,6 +500,11 @@ bool mi_remove_attr(struct mft_inode *mi, struct ATTRIB *attr) > >> if (aoff + asize > used) > >> return false; > >> > >> + if (ni && is_attr_indexed(attr)) { > >> + le16_add_cpu(&ni->mi.mrec->hard_links, -1); > >> + ni->mi.dirty = true; > >> + } > >> + > >> used -= asize; > >> memmove(attr, Add2Ptr(attr, asize), used - aoff); > >> rec->used = cpu_to_le32(used); > >> diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c > >> index b4c921e4bc1a..22fd5eb32c5b 100644 > >> --- a/fs/ntfs3/xattr.c > >> +++ b/fs/ntfs3/xattr.c > >> @@ -395,11 +395,13 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name, > >> } > >> > >> err = ni_insert_resident(ni, sizeof(struct EA_INFO), > >> - ATTR_EA_INFO, NULL, 0, NULL, NULL); > >> + ATTR_EA_INFO, NULL, 0, NULL, NULL, > >> + NULL); > >> if (err) > >> goto out; > >> > >> - err = ni_insert_resident(ni, 0, ATTR_EA, NULL, 0, NULL, NULL); > >> + err = ni_insert_resident(ni, 0, ATTR_EA, NULL, 0, NULL, NULL, > >> + NULL); > >> if (err) > >> goto out; > >> } > >> @@ -419,9 +421,7 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name, > >> > >> if (!size) { > >> /* Delete xattr, ATTR_EA_INFO */ > >> - err = ni_remove_attr_le(ni, attr, le); > >> - if (err) > >> - goto out; > >> + ni_remove_attr_le(ni, attr, mi, le); > >> } else { > >> p = resident_data_ex(attr, sizeof(struct EA_INFO)); > >> if (!p) { > >> @@ -441,9 +441,7 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name, > >> > >> if (!size) { > >> /* Delete xattr, ATTR_EA */ > >> - err = ni_remove_attr_le(ni, attr, le); > >> - if (err) > >> - goto out; > >> + ni_remove_attr_le(ni, attr, mi, le); > >> } else if (attr->non_res) { > >> err = ntfs_sb_write_run(sbi, &ea_run, 0, ea_all, size); > >> if (err) > >> @@ -605,8 +603,7 @@ static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns, > >> goto out; > >> } > >> > >> - err = ntfs_set_ea(inode, name, name_len, value, size, > >> - acl ? 0 : XATTR_REPLACE, locked); > >> + err = ntfs_set_ea(inode, name, name_len, value, size, 0, locked); > >> if (!err) > >> set_cached_acl(inode, type, acl); > >> > >> @@ -632,8 +629,10 @@ static int ntfs_xattr_get_acl(struct user_namespace *mnt_userns, > >> struct posix_acl *acl; > >> int err; > >> > >> - if (!(inode->i_sb->s_flags & SB_POSIXACL)) > >> + if (!(inode->i_sb->s_flags & SB_POSIXACL)) { > >> + ntfs_inode_warn(inode, "add mount option \"acl\" to use acl"); > >> return -EOPNOTSUPP; > >> + } > >> > >> acl = ntfs_get_acl(inode, type); > >> if (IS_ERR(acl)) > >> @@ -655,8 +654,10 @@ static int ntfs_xattr_set_acl(struct user_namespace *mnt_userns, > >> struct posix_acl *acl; > >> int err; > >> > >> - if (!(inode->i_sb->s_flags & SB_POSIXACL)) > >> + if (!(inode->i_sb->s_flags & SB_POSIXACL)) { > >> + ntfs_inode_warn(inode, "add mount option \"acl\" to use acl"); > >> return -EOPNOTSUPP; > >> + } > >> > >> if (!inode_owner_or_capable(mnt_userns, inode)) > >> return -EPERM; > >> -- > >> 2.28.0 > >> ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-09-02 8:25 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <c076bd59-64b1-149b-3a23-5e89a77b7795@paragon-software.com>
2021-08-31 21:54 ` [PATCH] fs/ntfs3: Rework file operations Kari Argillander
2021-09-01 16:54 ` Konstantin Komarov
2021-09-02 8:25 ` Kari Argillander
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).