linux-cifs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Christoph Hellwig <hch@infradead.org>
To: Namjae Jeon <namjae.jeon@samsung.com>
Cc: linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-cifs@vger.kernel.org, smfrench@gmail.com,
	stfrench@microsoft.com, willy@infradead.org,
	aurelien.aptel@gmail.com,
	linux-cifsd-devel@lists.sourceforge.net,
	senozhatsky@chromium.org, sandeen@sandeen.net, aaptel@suse.com,
	hch@infradead.org, viro@zeniv.linux.org.uk,
	ronniesahlberg@gmail.com, hch@lst.de, dan.carpenter@oracle.com,
	Sergey Senozhatsky <sergey.senozhatsky@gmail.com>,
	Hyunchul Lee <hyc.lee@gmail.com>
Subject: Re: [PATCH v4 08/10] cifsd: add file operations
Date: Tue, 15 Jun 2021 09:47:23 +0100	[thread overview]
Message-ID: <YMhpG/sAjO3WKKc3@infradead.org> (raw)
In-Reply-To: <20210602034847.5371-9-namjae.jeon@samsung.com>

On Wed, Jun 02, 2021 at 12:48:45PM +0900, Namjae Jeon wrote:
> +#include <linux/rwlock.h>
> +
> +#include "glob.h"
> +#include "buffer_pool.h"
> +#include "connection.h"
> +#include "mgmt/ksmbd_ida.h"
> +
> +static struct kmem_cache *filp_cache;
> +
> +struct wm {
> +	struct list_head	list;
> +	unsigned int		sz;
> +	char			buffer[0];

This should use buffer[];

> +};
> +
> +struct wm_list {
> +	struct list_head	list;
> +	unsigned int		sz;
> +
> +	spinlock_t		wm_lock;
> +	int			avail_wm;
> +	struct list_head	idle_wm;
> +	wait_queue_head_t	wm_wait;
> +};

What does wm stand for?

This looks like arbitrary caching of vmalloc buffers.  I thought we
decided to just make vmalloc suck less rather than papering over that?

> +static LIST_HEAD(wm_lists);
> +static DEFINE_RWLOCK(wm_lists_lock);

Especially as this isn't going to scale at all using global loists and
locks.

> +void ksmbd_free_file_struct(void *filp)
> +{
> +	kmem_cache_free(filp_cache, filp);
> +}
> +
> +void *ksmbd_alloc_file_struct(void)
> +{
> +	return kmem_cache_zalloc(filp_cache, GFP_KERNEL);
> +}

These are only ued in vfs_cache.c . So I'd suggest to just move
filp_cache there and drop these wrappers.

> +}
> +
> +static void ksmbd_vfs_inherit_owner(struct ksmbd_work *work,
> +				    struct inode *parent_inode,
> +				    struct inode *inode)
> +{
> +	if (!test_share_config_flag(work->tcon->share_conf,
> +				    KSMBD_SHARE_FLAG_INHERIT_OWNER))
> +		return;
> +
> +	i_uid_write(inode, i_uid_read(parent_inode));
> +}

Can you explain this a little more?  When do the normal create/mdir
fail to inherit the owner?

int ksmbd_vfs_inode_permission(struct dentry *dentry, int acc_mode, bool delete)
> +{
> +	int mask, ret = 0;
> +
> +	mask = 0;
> +	acc_mode &= O_ACCMODE;
> +
> +	if (acc_mode == O_RDONLY)
> +		mask = MAY_READ;
> +	else if (acc_mode == O_WRONLY)
> +		mask = MAY_WRITE;
> +	else if (acc_mode == O_RDWR)
> +		mask = MAY_READ | MAY_WRITE;

How about already setting up the MAY_ flags in smb2_create_open_flags
and returning them in extra argument?  That keeps the sm to Linux
translation in a single place.

> +
> +	if (inode_permission(&init_user_ns, d_inode(dentry), mask | MAY_OPEN))
> +		return -EACCES;

And this call can be open coded in the only caller.

> +	if (delete) {

And this block could be split into a nice self-contained helper.

> +		struct dentry *child, *parent;
> +
> +		parent = dget_parent(dentry);
> +		inode_lock_nested(d_inode(parent), I_MUTEX_PARENT);
> +		child = lookup_one_len(dentry->d_name.name, parent,
> +				       dentry->d_name.len);
> +		if (IS_ERR(child)) {
> +			ret = PTR_ERR(child);
> +			goto out_lock;
> +		}
> +
> +		if (child != dentry) {
> +			ret = -ESTALE;
> +			dput(child);
> +			goto out_lock;
> +		}
> +		dput(child);

That being said I do not understand the need for this re-lookup at all.

> +	if (!inode_permission(&init_user_ns, d_inode(dentry), MAY_OPEN | MAY_WRITE))

All these inode_permission lines have overly long lines.  It might be
worth to pass the user_namespace to this function, not only to shorten
the code, but also to prepare for user namespace support.

> +	parent = dget_parent(dentry);
> +	inode_lock_nested(d_inode(parent), I_MUTEX_PARENT);
> +	child = lookup_one_len(dentry->d_name.name, parent,
> +			       dentry->d_name.len);
> +	if (IS_ERR(child)) {
> +		ret = PTR_ERR(child);
> +		goto out_lock;
> +	}
> +
> +	if (child != dentry) {
> +		ret = -ESTALE;
> +		dput(child);
> +		goto out_lock;
> +	}
> +	dput(child);

This is the same weird re-lookup dance as above.  IFF there is a good
rationale for it it needs to go into a self-contained and well
documented helper.

> +int ksmbd_vfs_create(struct ksmbd_work *work, const char *name, umode_t mode)
> +{
> +	struct path path;
> +	struct dentry *dentry;
> +	int err;
> +
> +	dentry = kern_path_create(AT_FDCWD, name, &path, 0);

Curious:  why is this using absolute or CWD based path instead of
doing lookups based off the parent as done by e.g. nfsd?  Same also
for mkdir and co.

> +{
> +	struct file *filp;
> +	ssize_t nbytes = 0;
> +	char *rbuf;
> +	struct inode *inode;
> +
> +	rbuf = work->aux_payload_buf;
> +	filp = fp->filp;
> +	inode = file_inode(filp);

These can be initialized on the declaration lines to make the code a
little easier to read.

> +	if (!work->tcon->posix_extensions) {
> +		spin_lock(&src_dent->d_lock);
> +		list_for_each_entry(dst_dent, &src_dent->d_subdirs, d_child) {
> +			struct ksmbd_file *child_fp;
> +
> +			if (d_really_is_negative(dst_dent))
> +				continue;
> +
> +			child_fp = ksmbd_lookup_fd_inode(d_inode(dst_dent));
> +			if (child_fp) {
> +				spin_unlock(&src_dent->d_lock);
> +				ksmbd_debug(VFS, "Forbid rename, sub file/dir is in use\n");
> +				return -EACCES;
> +			}
> +		}
> +		spin_unlock(&src_dent->d_lock);
> +	}

This begs for being split into a self-contained helper.

> +int ksmbd_vfs_lock(struct file *filp, int cmd, struct file_lock *flock)
> +{
> +	ksmbd_debug(VFS, "calling vfs_lock_file\n");
> +	return vfs_lock_file(filp, cmd, flock, NULL);
> +}
> +
> +int ksmbd_vfs_readdir(struct file *file, struct ksmbd_readdir_data *rdata)
> +{
> +	return iterate_dir(file, &rdata->ctx);
> +}
> +
> +int ksmbd_vfs_alloc_size(struct ksmbd_work *work, struct ksmbd_file *fp,
> +			 loff_t len)
> +{
> +	smb_break_all_levII_oplock(work, fp, 1);
> +	return vfs_fallocate(fp->filp, FALLOC_FL_KEEP_SIZE, 0, len);
> +}

Please avoid such trivial wrappers that just make the code hard to
follow.

> +int ksmbd_vfs_fqar_lseek(struct ksmbd_file *fp, loff_t start, loff_t length,
> +			 struct file_allocated_range_buffer *ranges,
> +			 int in_count, int *out_count)

What is fqar?

> +
> +/*
> + * ksmbd_vfs_get_logical_sector_size() - get logical sector size from inode
> + * @inode: inode
> + *
> + * Return: logical sector size
> + */
> +unsigned short ksmbd_vfs_logical_sector_size(struct inode *inode)
> +{
> +	struct request_queue *q;
> +	unsigned short ret_val = 512;
> +
> +	if (!inode->i_sb->s_bdev)
> +		return ret_val;
> +
> +	q = inode->i_sb->s_bdev->bd_disk->queue;
> +
> +	if (q && q->limits.logical_block_size)
> +		ret_val = q->limits.logical_block_size;
> +
> +	return ret_val;

I don't think a CIFS server has any business poking into the block
layer.  What is this trying to do?

> +struct posix_acl *ksmbd_vfs_posix_acl_alloc(int count, gfp_t flags)
> +{
> +#if IS_ENABLED(CONFIG_FS_POSIX_ACL)
> +	return posix_acl_alloc(count, flags);
> +#else
> +	return NULL;
> +#endif
> +}
> +
> +struct posix_acl *ksmbd_vfs_get_acl(struct inode *inode, int type)
> +{
> +#if IS_ENABLED(CONFIG_FS_POSIX_ACL)
> +	return get_acl(inode, type);
> +#else
> +	return NULL;
> +#endif
> +}
> +
> +int ksmbd_vfs_set_posix_acl(struct inode *inode, int type,
> +			    struct posix_acl *acl)
> +{
> +#if IS_ENABLED(CONFIG_FS_POSIX_ACL)
> +	return set_posix_acl(&init_user_ns, inode, type, acl);
> +#else
> +	return -EOPNOTSUPP;
> +#endif
> +}

Please avoid these pointless wrappers and just use large code block
ifdefs or IS_ENABLED checks.

> +int ksmbd_vfs_copy_file_range(struct file *file_in, loff_t pos_in,
> +			      struct file *file_out, loff_t pos_out, size_t len)
> +{
> +	struct inode *inode_in = file_inode(file_in);
> +	struct inode *inode_out = file_inode(file_out);
> +	int ret;
> +
> +	ret = vfs_copy_file_range(file_in, pos_in, file_out, pos_out, len, 0);
> +	/* do splice for the copy between different file systems */
> +	if (ret != -EXDEV)
> +		return ret;
> +
> +	if (S_ISDIR(inode_in->i_mode) || S_ISDIR(inode_out->i_mode))
> +		return -EISDIR;
> +	if (!S_ISREG(inode_in->i_mode) || !S_ISREG(inode_out->i_mode))
> +		return -EINVAL;
> +
> +	if (!(file_in->f_mode & FMODE_READ) ||
> +	    !(file_out->f_mode & FMODE_WRITE))
> +		return -EBADF;
> +
> +	if (len == 0)
> +		return 0;
> +
> +	file_start_write(file_out);
> +
> +	/*
> +	 * skip the verification of the range of data. it will be done
> +	 * in do_splice_direct
> +	 */
> +	ret = do_splice_direct(file_in, &pos_in, file_out, &pos_out,
> +			       len > MAX_RW_COUNT ? MAX_RW_COUNT : len, 0);

vfs_copy_file_range already does this type of fallback, so this is dead
code.

> +#define XATTR_NAME_STREAM_LEN		(sizeof(XATTR_NAME_STREAM) - 1)
> +
> +enum {
> +	XATTR_DOSINFO_ATTRIB		= 0x00000001,
> +	XATTR_DOSINFO_EA_SIZE		= 0x00000002,
> +	XATTR_DOSINFO_SIZE		= 0x00000004,
> +	XATTR_DOSINFO_ALLOC_SIZE	= 0x00000008,
> +	XATTR_DOSINFO_CREATE_TIME	= 0x00000010,
> +	XATTR_DOSINFO_CHANGE_TIME	= 0x00000020,
> +	XATTR_DOSINFO_ITIME		= 0x00000040
> +};
> +
> +struct xattr_dos_attrib {
> +	__u16	version;
> +	__u32	flags;
> +	__u32	attr;
> +	__u32	ea_size;
> +	__u64	size;
> +	__u64	alloc_size;
> +	__u64	create_time;
> +	__u64	change_time;
> +	__u64	itime;
> +};

These looks like on-disk structures.  Any chance you could re-order
the headers so that things like on-disk, on the wire and netlink uapi
structures all have a dedicated and well documented header for
themselves?

> +	read_lock(&ci->m_lock);
> +	list_for_each(cur, &ci->m_fp_list) {
> +		lfp = list_entry(cur, struct ksmbd_file, node);

Please use list_for_each_entry.  There are very few places left where
using list_for_each makes sense.

> +		if (inode == FP_INODE(lfp)) {
> +			atomic_dec(&ci->m_count);
> +			read_unlock(&ci->m_lock);
> +			return lfp;
> +		}
> +	}
> +	atomic_dec(&ci->m_count);
> +	read_unlock(&ci->m_lock);

So a successful find increments m_count, but a miss decreases it?
Isn't this going to create an underflow?

> +	if (!fp->f_ci) {
> +		ksmbd_free_file_struct(fp);
> +		return ERR_PTR(-ENOMEM);
> +	}
> +
> +	ret = __open_id(&work->sess->file_table, fp, OPEN_ID_TYPE_VOLATILE_ID);
> +	if (ret) {
> +		ksmbd_inode_put(fp->f_ci);
> +		ksmbd_free_file_struct(fp);
> +		return ERR_PTR(ret);
> +	}
> +
> +	atomic_inc(&work->conn->stats.open_files_count);
> +	return fp;

Please use goto based unwinding instead of duplicating the resoure
cleanup.

> +static bool tree_conn_fd_check(struct ksmbd_tree_connect *tcon, struct ksmbd_file *fp)

Overly long line.

> +{
> +	return fp->tcon != tcon;
> +}
> +
> +static bool session_fd_check(struct ksmbd_tree_connect *tcon, struct ksmbd_file *fp)

Same.

  reply	other threads:[~2021-06-15  8:48 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <CGME20210602035813epcas1p49ba4ff37fe4b784aa76dce67e4036227@epcas1p4.samsung.com>
2021-06-02  3:48 ` [PATCH v4 00/10] cifsd: introduce new SMB3 kernel server Namjae Jeon
     [not found]   ` <CGME20210602035814epcas1p3f244e5e018b0d7164081399262bd4bb7@epcas1p3.samsung.com>
2021-06-02  3:48     ` [PATCH v4 01/10] cifsd: add document Namjae Jeon
     [not found]   ` <CGME20210602035815epcas1p18f19c96ea3d299f97a90d818b83a3c85@epcas1p1.samsung.com>
2021-06-02  3:48     ` [PATCH v4 02/10] cifsd: add server handler Namjae Jeon
2021-06-15  8:10       ` Christoph Hellwig
2021-06-16  6:49         ` Namjae Jeon
     [not found]   ` <CGME20210602035817epcas1p1a58f64bc2881ed37ed1fa70140d5bda1@epcas1p1.samsung.com>
2021-06-02  3:48     ` [PATCH v4 04/10] cifsd: add authentication Namjae Jeon
     [not found]   ` <CGME20210602035819epcas1p201c9ed34d8be214299db2684bdba706b@epcas1p2.samsung.com>
2021-06-02  3:48     ` [PATCH v4 07/10] cifsd: add oplock/lease cache mechanism Namjae Jeon
     [not found]   ` <CGME20210602035820epcas1p3c444b34a6b6a4252c9091e0bf6c0c167@epcas1p3.samsung.com>
2021-06-02  3:48     ` [PATCH v4 08/10] cifsd: add file operations Namjae Jeon
2021-06-15  8:47       ` Christoph Hellwig [this message]
2021-06-16  6:48         ` Namjae Jeon
     [not found]   ` <CGME20210602035821epcas1p2bb9cbdcdbd61fe9c66697e944c53f297@epcas1p2.samsung.com>
2021-06-02  3:48     ` [PATCH v4 09/10] cifsd: add Kconfig and Makefile Namjae Jeon
     [not found]   ` <CGME20210602035822epcas1p4013fc24d42e1ea13ef5d4f4b50751168@epcas1p4.samsung.com>
2021-06-02  3:48     ` [PATCH v4 10/10] MAINTAINERS: add cifsd kernel server Namjae Jeon
     [not found]   ` <CGME20210602035816epcas1p240598e76247034278ad45dc0827b2be7@epcas1p2.samsung.com>
     [not found]     ` <20210602034847.5371-4-namjae.jeon@samsung.com>
2021-06-15  8:11       ` [PATCH v4 03/10] cifsd: add trasport layers Christoph Hellwig
2021-06-16  6:49         ` Namjae Jeon

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=YMhpG/sAjO3WKKc3@infradead.org \
    --to=hch@infradead.org \
    --cc=aaptel@suse.com \
    --cc=aurelien.aptel@gmail.com \
    --cc=dan.carpenter@oracle.com \
    --cc=hch@lst.de \
    --cc=hyc.lee@gmail.com \
    --cc=linux-cifs@vger.kernel.org \
    --cc=linux-cifsd-devel@lists.sourceforge.net \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=namjae.jeon@samsung.com \
    --cc=ronniesahlberg@gmail.com \
    --cc=sandeen@sandeen.net \
    --cc=senozhatsky@chromium.org \
    --cc=sergey.senozhatsky@gmail.com \
    --cc=smfrench@gmail.com \
    --cc=stfrench@microsoft.com \
    --cc=viro@zeniv.linux.org.uk \
    --cc=willy@infradead.org \
    --subject='Re: [PATCH v4 08/10] cifsd: add file operations' \
    /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

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).