From: Boaz Harrosh <boaz@plexistor.com>
To: "Schumaker, Anna" <Anna.Schumaker@netapp.com>,
"linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
"viro@zeniv.linux.org.uk" <viro@zeniv.linux.org.uk>,
"mbenjami@redhat.com" <mbenjami@redhat.com>
Cc: "dan.j.williams@intel.com" <dan.j.williams@intel.com>,
"mszeredi@redhat.com" <mszeredi@redhat.com>,
"willy@infradead.org" <willy@infradead.org>,
"amir73il@gmail.com" <amir73il@gmail.com>,
"Manole, Sagi" <Sagi.Manole@netapp.com>
Subject: Re: [PATCH 11/16] zuf: Write/Read implementation
Date: Thu, 14 Nov 2019 17:15:34 +0200 [thread overview]
Message-ID: <46507231-91ba-0597-94f8-48f00da46077@plexistor.com> (raw)
In-Reply-To: <db90d73233484d251755c5a0cb7ee570b3fc9d19.camel@netapp.com>
On 29/10/2019 22:08, Schumaker, Anna wrote:
> Hi Boaz,
>
> On Thu, 2019-09-26 at 05:07 +0300, Boaz Harrosh wrote:
>> zufs Has two ways to do IO.
<>
>> +static int rw_overflow_handler(struct zuf_dispatch_op *zdo, void *arg,
>> + ulong max_bytes)
>> +{
>> + struct zufs_ioc_IO *io = container_of(zdo->hdr, typeof(*io), hdr);
This one is setting the typed pointer @io to be the same of what @zdo->hdr is
>> + struct zufs_ioc_IO *io_user = arg;
>> + int err;
>> +
>> + *io = *io_user;
This one is deep copying the full size structure pointed to by io_user
to the space pointed to by io. (same as zdo->hdr)
Same as memcpy(io, io_user, sizeof(*io))
>
> It looks like you're setting *io using the container_of() macro a few lines above, and then
> overwriting it here without ever using it. Can you remove one of these to make it clearer which
> one you meant to use?
>
These are not redundant its the confusing C thing where declarations
of pointers + assignment means the pointer and not the content.
This code is correct
>> +
>> + err = _ioc_bounds_check(&io->ziom, &io_user->ziom, arg + max_bytes);
>> + if (unlikely(err))
>> + return err;
>> +
>> + if ((io->hdr.err == -EZUFS_RETRY) &&
>> + io->ziom.iom_n && _zufs_iom_pop(io->iom_e)) {
>> +
>> + zuf_dbg_rw(
>> + "[%s]zuf_iom_execute_sync(%d) max=0x%lx iom_e[%d] => %d\n",
>> + zuf_op_name(io->hdr.operation), io->ziom.iom_n,
>> + max_bytes, _zufs_iom_opt_type(io_user->iom_e),
>> + io->hdr.err);
>> +
>> + io->hdr.err = zuf_iom_execute_sync(zdo->sb, zdo->inode,
>> + io_user->iom_e,
>> + io->ziom.iom_n);
>> + return EZUF_RETRY_DONE;
>> + }
<>
>> +static ssize_t _IO_gm(struct zuf_sb_info *sbi, struct inode *inode,
>> + ulong *on_stack, uint max_on_stack,
>> + struct iov_iter *ii, struct kiocb *kiocb,
>> + struct file_ra_state *ra, uint rw)
>> +{
>> + ssize_t size = 0;
>> + ssize_t ret = 0;
>> + enum big_alloc_type bat;
>> + ulong *bns;
>> + uint max_bns = min_t(uint,
>> + md_o2p_up(iov_iter_count(ii) + (kiocb->ki_pos & ~PAGE_MASK)),
>> + ZUS_API_MAP_MAX_PAGES);
>> +
>> + bns = big_alloc(max_bns * sizeof(ulong), max_on_stack, on_stack,
>> + GFP_NOFS, &bat);
>> + if (unlikely(!bns)) {
>> + zuf_err("life was more simple on the stack max_bns=%d\n",
>> + max_bns);
>> + return -ENOMEM;
>> + }
>> +
>> + while (iov_iter_count(ii)) {
>> + ret = _IO_gm_inner(sbi, inode, bns, max_bns, ii, ra,
>> + kiocb->ki_pos, rw);
>> + if (unlikely(ret < 0))
>> + break;
>> +
>> + kiocb->ki_pos += ret;
>> + size += ret;
>> + }
>> +
>> + big_free(bns, bat);
>> +
>> + return size ?: ret;
>
> It looks like you're returning "ret" if the ternary evaluates to false, but it's not clear to
> me what is returned if it evaluates to true. It's possible it's okay, but I just don't know
> enough about how ternaries work in this case.
>
Yes Thanks, Will fix. Not suppose to use this in the Kernel.
>> +}
>> +
<>
>> +int zuf_iom_execute_sync(struct super_block *sb, struct inode *inode,
>> + __u64 *iom_e_user, uint iom_n)
>> +{
>> + struct zuf_sb_info *sbi = SBI(sb);
>> + struct t2_io_state rd_tis = {};
>> + struct t2_io_state wr_tis = {};
>> + struct _iom_exec_info iei = {};
>> + int err, err_r, err_w;
>> +
>> + t2_io_begin(sbi->md, READ, NULL, 0, -1, &rd_tis);
>> + t2_io_begin(sbi->md, WRITE, NULL, 0, -1, &wr_tis);
>> +
>> + iei.sb = sb;
>> + iei.inode = inode;
>> + iei.rd_tis = &rd_tis;
>> + iei.wr_tis = &wr_tis;
>> + iei.iom_e = iom_e_user;
>> + iei.iom_n = iom_n;
>> + iei.print = 0;
>> +
>> + err = _iom_execute_inline(&iei);
>> +
>> + err_r = t2_io_end(&rd_tis, true);
>> + err_w = t2_io_end(&wr_tis, true);
>> +
>> + /* TODO: not sure if OK when _iom_execute return with -ENOMEM
>> + * In such a case, we might be better of skiping t2_io_ends.
>> + */
>> + return err ?: (err_r ?: err_w);
>
> Same question here.
>
> Thanks,
> Anna
>
Yes Will fix
Thanks Anna
Can I put Reviewed-by on this patch?
>> +}
Much obliged
Boaz
next prev parent reply other threads:[~2019-11-14 15:15 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-09-26 2:07 [PATCHSET v02 00/16] zuf: ZUFS Zero-copy User-mode FileSystem Boaz Harrosh
2019-09-26 2:07 ` [PATCH 01/16] fs: Add the ZUF filesystem to the build + License Boaz Harrosh
2019-09-26 2:07 ` [PATCH 02/16] MAINTAINERS: Add the ZUFS maintainership Boaz Harrosh
2019-09-26 2:07 ` [PATCH 03/16] zuf: Preliminary Documentation Boaz Harrosh
2019-09-26 2:07 ` [PATCH 04/16] zuf: zuf-rootfs Boaz Harrosh
2019-09-26 2:07 ` [PATCH 05/16] zuf: zuf-core The ZTs Boaz Harrosh
2019-09-26 2:07 ` [PATCH 06/16] zuf: Multy Devices Boaz Harrosh
2019-09-26 2:07 ` [PATCH 07/16] zuf: mounting Boaz Harrosh
2019-09-26 2:07 ` [PATCH 08/16] zuf: Namei and directory operations Boaz Harrosh
2019-09-26 2:07 ` [PATCH 09/16] zuf: readdir operation Boaz Harrosh
2019-09-26 2:07 ` [PATCH 10/16] zuf: symlink Boaz Harrosh
2019-09-26 2:07 ` [PATCH 11/16] zuf: Write/Read implementation Boaz Harrosh
[not found] ` <db90d73233484d251755c5a0cb7ee570b3fc9d19.camel@netapp.com>
2019-10-29 20:15 ` Matthew Wilcox
2019-11-14 14:04 ` Boaz Harrosh
2019-11-14 15:15 ` Boaz Harrosh [this message]
2019-11-14 16:08 ` Schumaker, Anna
2019-09-26 2:07 ` [PATCH 12/16] zuf: mmap & sync Boaz Harrosh
2019-09-26 2:07 ` [PATCH 13/16] zuf: More file operation Boaz Harrosh
2019-09-26 2:07 ` [PATCH 14/16] zuf: ioctl implementation Boaz Harrosh
2019-09-26 2:07 ` [PATCH 15/16] zuf: xattr && acl implementation Boaz Harrosh
2019-09-26 2:07 ` [PATCH 16/16] zuf: Support for dynamic-debug of zusFSs Boaz Harrosh
2019-09-26 7:11 ` [PATCHSET v02 00/16] zuf: ZUFS Zero-copy User-mode FileSystem Miklos Szeredi
2019-09-26 9:41 ` Bernd Schubert
2019-09-26 11:27 ` Boaz Harrosh
2019-09-26 12:12 ` Bernd Schubert
2019-09-26 12:24 ` Boaz Harrosh
2019-09-26 13:45 ` Miklos Szeredi
2019-09-26 12:48 ` Boaz Harrosh
2019-09-26 13:48 ` Miklos Szeredi
2019-09-26 11:41 ` Boaz Harrosh
-- strict thread matches above, loose matches on Subject: below --
2019-08-12 16:47 [PATCHSET " Boaz Harrosh
2019-08-12 16:48 ` [PATCH 11/16] zuf: Write/Read implementation Boaz Harrosh
2019-08-12 16:42 [PATCHSET 00/16] zuf: ZUFS Zero-copy User-mode FileSystem Boaz Harrosh
2019-08-12 16:42 ` [PATCH 11/16] zuf: Write/Read implementation Boaz Harrosh
2019-08-13 10:23 ` kbuild test robot
2019-08-13 10:28 ` Boaz Harrosh
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=46507231-91ba-0597-94f8-48f00da46077@plexistor.com \
--to=boaz@plexistor.com \
--cc=Anna.Schumaker@netapp.com \
--cc=Sagi.Manole@netapp.com \
--cc=amir73il@gmail.com \
--cc=dan.j.williams@intel.com \
--cc=linux-fsdevel@vger.kernel.org \
--cc=mbenjami@redhat.com \
--cc=mszeredi@redhat.com \
--cc=viro@zeniv.linux.org.uk \
--cc=willy@infradead.org \
/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).