Re: [PATCH v2] ksmbd: remove unnecessary generic_fillattr in smb2_open

From: Hyunchul Lee <hyc.lee@gmail.com>
To: Namjae Jeon <linkinjeon@kernel.org>
Cc: linux-cifs <linux-cifs@vger.kernel.org>,
	Steve French <smfrench@gmail.com>,
	Sergey Senozhatsky <senozhatsky@chromium.org>
Subject: Re: [PATCH v2] ksmbd: remove unnecessary generic_fillattr in smb2_open
Date: Thu, 11 Aug 2022 14:32:38 +0900	[thread overview]
Message-ID: <CANFS6bZ3EqBhFCHU=Bhgx+CVMg4Ug7orQSYTagKXCVrLezRxUQ@mail.gmail.com> (raw)
In-Reply-To: <CANFS6bZd5E395Vdfa8Wh2DZkFdxgUxsO=M4kCEpJ2s6OUKW73A@mail.gmail.com>

2022년 8월 11일 (목) 오전 8:18, Hyunchul Lee <hyc.lee@gmail.com>님이 작성:
>
> 2022년 8월 10일 (수) 오후 5:00, Namjae Jeon <linkinjeon@kernel.org>님이 작성:
> >
> > 2022-08-10 16:58 GMT+09:00, Namjae Jeon <linkinjeon@kernel.org>:
> > > 2022-08-10 10:04 GMT+09:00, Hyunchul Lee <hyc.lee@gmail.com>:
> > >> Move the call of ksmbd_vfs_getattr above the place
> > >> where stat is needed, and remove unnecessary
> > >> the call of generic_fillattr.
> > >>
> > >> This patch fixes wrong AllocationSize of SMB2_CREATE
> > >> response. Because ext4 updates inode->i_blocks only
> > >> when disk space is allocated, generic_fillattr does
> > >> not set stat.blocks properly for delayed allocation.
> > And how can I reproduce problem and make sure that it is improved?
>
> This issue can be reproduce with commands below:
>
> touch ${FILENAME}
> xfs_io -c "pwrite -S 0xAB 0 40k" ${FILENAME}
> xfs_io -c "stat" ${FILENAME}
>
> 40KB are written, but the number of blocks are 8
> stat.size = 40960
> stat.blocks = 8
>
> > > So what causes delay allocation between the lines you moved this code?
>
> For the example above, the second command writes 40KB but
> inode->i_blocks is not  updated because of delayed allocation.
> And when the third command is executed, in smb2_open
> ksmbd_vfs_getattr returns stat.blocks which is 80, but
> the following generic_fillattr returns stat.blocks which
> 8.
>
> With this patch applied, the third command returns stat.blocks
> which is 88, not 80.
> I will look into this.
>

This is not a problem and happens only when acl_xattr config is enabled.
If the config is enabled. The security descriptor is written to xattr
as not inline,
and the blocks for xattr are counted.

>
>
> > >
> > >> But ext4 returns the blocks that include the delayed
> > >> allocation blocks when getattr is called.
> > >>
> > >> Signed-off-by: Hyunchul Lee <hyc.lee@gmail.com>
> > >> ---
> > >> Changes from v1:
> > >>  - Update the commit description.
> > >>
> > >>  fs/ksmbd/smb2pdu.c | 15 ++++++---------
> > >>  1 file changed, 6 insertions(+), 9 deletions(-)
> > >>
> > >> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
> > >> index e6f4ccc12f49..7b4bd0d81133 100644
> > >> --- a/fs/ksmbd/smb2pdu.c
> > >> +++ b/fs/ksmbd/smb2pdu.c
> > >> @@ -3022,12 +3022,6 @@ int smb2_open(struct ksmbd_work *work)
> > >>      list_add(&fp->node, &fp->f_ci->m_fp_list);
> > >>      write_unlock(&fp->f_ci->m_lock);
> > >>
> > >> -    rc = ksmbd_vfs_getattr(&path, &stat);
> > >> -    if (rc) {
> > >> -            generic_fillattr(user_ns, d_inode(path.dentry), &stat);
> > >> -            rc = 0;
> > >> -    }
> > >> -
> > >>      /* Check delete pending among previous fp before oplock break */
> > >>      if (ksmbd_inode_pending_delete(fp)) {
> > >>              rc = -EBUSY;
> > >> @@ -3114,6 +3108,12 @@ int smb2_open(struct ksmbd_work *work)
> > >>              }
> > >>      }
> > >>
> > >> +    rc = ksmbd_vfs_getattr(&path, &stat);
> > >> +    if (rc) {
> > >> +            generic_fillattr(user_ns, d_inode(path.dentry), &stat);
> > >> +            rc = 0;
> > >> +    }
> > >> +
> > >>      if (stat.result_mask & STATX_BTIME)
> > >>              fp->create_time = ksmbd_UnixTimeToNT(stat.btime);
> > >>      else
> > >> @@ -3129,9 +3129,6 @@ int smb2_open(struct ksmbd_work *work)
> > >>
> > >>      memcpy(fp->client_guid, conn->ClientGUID, SMB2_CLIENT_GUID_SIZE);
> > >>
> > >> -    generic_fillattr(user_ns, file_inode(fp->filp),
> > >> -                     &stat);
> > >> -
> > >>      rsp->StructureSize = cpu_to_le16(89);
> > >>      rcu_read_lock();
> > >>      opinfo = rcu_dereference(fp->f_opinfo);
> > >> --
> > >> 2.17.1
> > >>
> > >>
> > >
>
>
>
> --
> Thanks,
> Hyunchul

-- 
Thanks,
Hyunchul