From: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
To: Kari Argillander <kari.argillander@gmail.com>
Cc: <ntfs3@lists.linux.dev>, <linux-kernel@vger.kernel.org>,
<linux-fsdevel@vger.kernel.org>
Subject: Re: [PATCH] fs/ntfs3: Rework file operations
Date: Wed, 1 Sep 2021 19:54:18 +0300 [thread overview]
Message-ID: <e20a1451-a8de-1c47-df7a-c281af71030c@paragon-software.com> (raw)
In-Reply-To: <20210831215410.wkolls3jvev2rebk@kari-VirtualBox>
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
>>
next prev parent reply other threads:[~2021-09-01 16:54 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-31 16:34 [PATCH] fs/ntfs3: Rework file operations Konstantin Komarov
2021-08-31 21:54 ` Kari Argillander
2021-09-01 16:54 ` Konstantin Komarov [this message]
2021-09-02 8:25 ` Kari Argillander
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e20a1451-a8de-1c47-df7a-c281af71030c@paragon-software.com \
--to=almaz.alexandrovich@paragon-software.com \
--cc=kari.argillander@gmail.com \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=ntfs3@lists.linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).