From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-18.3 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id C6930C4320A for ; Wed, 1 Sep 2021 16:54:29 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id A2F7161074 for ; Wed, 1 Sep 2021 16:54:29 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1345087AbhIAQzZ (ORCPT ); Wed, 1 Sep 2021 12:55:25 -0400 Received: from relaydlg-01.paragon-software.com ([81.5.88.159]:39258 "EHLO relaydlg-01.paragon-software.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1345048AbhIAQzT (ORCPT ); Wed, 1 Sep 2021 12:55:19 -0400 Received: from dlg2.mail.paragon-software.com (vdlg-exch-02.paragon-software.com [172.30.1.105]) by relaydlg-01.paragon-software.com (Postfix) with ESMTPS id 5E38D820C9; Wed, 1 Sep 2021 19:54:19 +0300 (MSK) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=paragon-software.com; s=mail; t=1630515259; bh=bzi2GLyOT2o8zB7HZk06fftgsjKubjbI2GU0hAqSQPM=; h=Subject:To:CC:References:From:Date:In-Reply-To; b=rRZIR2q+hBVvrRwKiZTA0FFYye1yAQM2oIPmamlpWMmbIwKAbbkchFxAaBH0ZJaY4 I9Tmu948+D9aqfGXdpQgAu9tLfPOCvj9yoKUFEZLvCyJ732zjGPgYKcvkkqaWTma57 AoWpCpR0VrPKF3cvPBy3lh9bDS1RelVt5CfpXWqU= Received: from [192.168.211.24] (192.168.211.24) by vdlg-exch-02.paragon-software.com (172.30.1.105) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2176.2; Wed, 1 Sep 2021 19:54:18 +0300 Subject: Re: [PATCH] fs/ntfs3: Rework file operations To: Kari Argillander CC: , , References: <20210831215410.wkolls3jvev2rebk@kari-VirtualBox> From: Konstantin Komarov Message-ID: Date: Wed, 1 Sep 2021 19:54:18 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.13.0 MIME-Version: 1.0 In-Reply-To: <20210831215410.wkolls3jvev2rebk@kari-VirtualBox> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 7bit X-Originating-IP: [192.168.211.24] X-ClientProxiedBy: vobn-exch-01.paragon-software.com (172.30.72.13) To vdlg-exch-02.paragon-software.com (172.30.1.105) Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >> --- >> 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 >>