Re: [PATCH v6 3/4] scatterlist: add sgl_compare_sgl() function
On Mon, 18 Jan 2021 20:04:20 -0500, Douglas Gilbert wrote: > >> +bool sgl_compare_sgl(struct scatterlist *x_sgl, unsigned int x_nents, > >> off_t x_skip, > >> + struct scatterlist *y_sgl, unsigned int y_nents, off_t > >> y_skip, > >> + size_t n_bytes); > >> + > >> +bool sgl_compare_sgl_idx(struct scatterlist *x_sgl, unsigned int x_nents, > >> off_t x_skip, > >> + struct scatterlist *y_sgl, unsigned int y_nents, off_t > >> y_skip, > >> + size_t n_bytes, size_t *miscompare_idx); > > > > > > This patch looks good and works fine as a replacement for > > compare_and_write_do_cmp(). One minor suggestion would be to name it > > sgl_equal() or similar, to perhaps better reflect the bool return and > > avoid memcmp() confusion. Either way: > > Reviewed-by: David Disseldorp > > Thanks. NVMe calls the command that does this Compare and SCSI uses > COMPARE AND WRITE (and VERIFY(BYTCHK=1) ) but "equal" is fine with me. > There will be another patchset version (at least) so there is time > to change. > > Do you want: >- sgl_equal(...), or >- sgl_equal_sgl(...) ? I'd probably prefer the former as it's shorter, but I don't feel strongly about it. The latter would make sense if you expect sgl compare helpers for other buffer types. Cheers, David
Re: [PATCH v6 3/4] scatterlist: add sgl_compare_sgl() function
On Mon, 18 Jan 2021 11:30:05 -0500, Douglas Gilbert wrote: > After enabling copies between scatter gather lists (sgl_s), another > storage related operation is to compare two sgl_s. This new function > is modelled on NVMe's Compare command and the SCSI VERIFY(BYTCHK=1) > command. Like memcmp() this function returns false on the first > miscompare and stops comparing. > > A helper function called sgl_compare_sgl_idx() is added. It takes an > additional parameter (miscompare_idx) which is a pointer. If that > pointer is non-NULL and a miscompare is detected (i.e. the function > returns false) then the byte index of the first miscompare is written > to *miscomapre_idx. Knowing the location of the first miscompare is > needed to implement the SCSI COMPARE AND WRITE command properly. > > Reviewed-by: Bodo Stroesser > Signed-off-by: Douglas Gilbert > --- > include/linux/scatterlist.h | 8 +++ > lib/scatterlist.c | 109 > 2 files changed, 117 insertions(+) > > diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h > index 3f836a3246aa..71be65f9ebb5 100644 > --- a/include/linux/scatterlist.h > +++ b/include/linux/scatterlist.h > @@ -325,6 +325,14 @@ size_t sgl_copy_sgl(struct scatterlist *d_sgl, unsigned > int d_nents, off_t d_ski > struct scatterlist *s_sgl, unsigned int s_nents, off_t > s_skip, > size_t n_bytes); > > +bool sgl_compare_sgl(struct scatterlist *x_sgl, unsigned int x_nents, off_t > x_skip, > + struct scatterlist *y_sgl, unsigned int y_nents, off_t > y_skip, > + size_t n_bytes); > + > +bool sgl_compare_sgl_idx(struct scatterlist *x_sgl, unsigned int x_nents, > off_t x_skip, > + struct scatterlist *y_sgl, unsigned int y_nents, off_t > y_skip, > + size_t n_bytes, size_t *miscompare_idx); This patch looks good and works fine as a replacement for compare_and_write_do_cmp(). One minor suggestion would be to name it sgl_equal() or similar, to perhaps better reflect the bool return and avoid memcmp() confusion. Either way: Reviewed-by: David Disseldorp Cheers, David
Re: Userspace regression with 6baca7601bde ("scsi: target: drop unused pi_prot_format attribute storage")
Hi Laura, Thanks for the report... On Sun, 3 Feb 2019 17:56:00 +0100, Laura Abbott wrote: > Fedora got a bug report of a new permission denied error with 5.0-rc2: > > > File "/usr/lib/python3.7/site-packages/rtslib_fb/utils.py", line 100, in > > fread > > with open(path, 'r') as file_fd: > > PermissionError: [Errno 13] Permission denied: > > '/sys/kernel/config/target/core/fileio_28/xxx/attrib/pi_prot_format' > > This looks like an intentional behavior change with > > commit 6baca7601bdee2e57f20c45d63eb53b89b33e816 > Author: David Disseldorp > Date: Fri Nov 23 18:36:11 2018 +0100 > > scsi: target: drop unused pi_prot_format attribute storage > > On write, the pi_prot_format configfs attribute invokes the device > format_prot() callback if present. Read dumps the contents of > se_dev_attrib.pi_prot_format which is always zero. Make the configfs > attribute write-only, and drop the always zero > se_dev_attrib.pi_prot_format > storage. > > Signed-off-by: David Disseldorp > Reviewed-by: Christoph Hellwig > Signed-off-by: Martin K. Petersen > > > Unfortunately, existing code that's opening with read permissions is now > broken. > Can this be reverted? Full bug at > https://bugzilla.redhat.com/show_bug.cgi?id=1667505 Lee (cc'ed) pinged me a couple of days ago about the same issue. My preference would be to add back a dummy read handler without the corresponding (unused) se_dev_attrib.pi_prot_format member. I'll prepare something tomorrow with this, but if it's urgent then I'd also be okay with a straight revert. Cheers, David
Re: [PATCH 1/1] target:separate tx/rx cmd_puds
On Thu, 5 Apr 2018 13:12:12 +0200, David Disseldorp wrote: > > -CONFIGFS_ATTR_RO(target_stat_tgt_port_, in_cmds); > > +CONFIGFS_ATTR_RO(target_stat_tgt_port_, tx_cmds); > > +CONFIGFS_ATTR_RO(target_stat_tgt_port_, rx_cmds); > > I don't think the in_cmds metric should be deleted here. It could be > calculated on the fly via tx_cmds + rx_cmds + nodata_cmds. @Zhang Zhuoyu: How about something like the following? https://git.samba.org/?p=ddiss/linux.git;a=commitdiff;h=73723ccf433424721830797d70cfb88d4596e0fc ...this keeps the in_cmds metric, and renames tx/rx_cmds read/write_cmds respectively. read/write_cmds is still a bit ambiguous, as it refers to the command data direction rather than SCSI READ/WRITE CDBs, but IMO it's clearer, and more consistent with the read/write_mbytes metrics. Cheers, David
Re: [PATCH 1/1] target:separate tx/rx cmd_puds
Hi, The commit summary has a typo (cmd_puds). That said, this change isn't iSCSI specific, so using "pdu" here doesn't make much sense IMO. On Wed, 21 Mar 2018 17:52:43 +0800, Zhang Zhuoyu wrote: > Separate tx/rx cmd_pdus in order to distinguish LUN read/write IOPS. > > Signed-off-by: Zhang Zhuoyu > --- > drivers/target/target_core_stat.c | 26 ++ > drivers/target/target_core_transport.c | 11 ++- > include/target/target_core_base.h | 3 ++- > 3 files changed, 34 insertions(+), 6 deletions(-) > > diff --git a/drivers/target/target_core_stat.c > b/drivers/target/target_core_stat.c > index f0db91e..9099494 100644 > --- a/drivers/target/target_core_stat.c > +++ b/drivers/target/target_core_stat.c > @@ -706,7 +722,8 @@ static ssize_t > target_stat_tgt_port_hs_in_cmds_show(struct config_item *item, > CONFIGFS_ATTR_RO(target_stat_tgt_port_, indx); > CONFIGFS_ATTR_RO(target_stat_tgt_port_, name); > CONFIGFS_ATTR_RO(target_stat_tgt_port_, port_index); > -CONFIGFS_ATTR_RO(target_stat_tgt_port_, in_cmds); > +CONFIGFS_ATTR_RO(target_stat_tgt_port_, tx_cmds); > +CONFIGFS_ATTR_RO(target_stat_tgt_port_, rx_cmds); I don't think the in_cmds metric should be deleted here. It could be calculated on the fly via tx_cmds + rx_cmds + nodata_cmds. Cheers, David
Re: cp --reflink and target file open flags
On Wed, 16 Oct 2013 18:36:19 -0500 Steve French wrote: > cp --reflink opens the target file for O_WRONLY before invoking the > (BTRFS) ioctl for clone file, but for copy offload over the network > the SMB2 specification requires that the target file be open O_RDWR. > > I may be able to upgrade the target file handle on the fly by > reopening it in cifs.ko, and of course I can write an SMB2/SMB3 > specific copy command, but it would be preferable to allow use of cp > --reflink since so many people are familiar with it. > > There is quite a bit of flexibility in server side copy offload - > more than cp an offer, especially when using SMB3 or later dialects > (e.g. in number of chunks sent at one time, chunk size, attributes > copied, and even whether to use T10 style offload), but still it would > be nice to support "cp --reflink" over the network. Any ideas on > this? > > After looking at copy.c in coreutils for cp - I couldn't think of any > trivial way to force cp to open the target RW. > > Ideas? You should be able to avoid this by using FSCTL_SRV_COPYCHUNK_WRITE instead of FSCTL_SRV_COPYCHUNK on the wire. The former doesn't require read access on the target, while the latter does. See [MS-SMB2] 2.2.31 and smbtorture's copy_chunk_bad_access test. Samba only supported FSCTL_SRV_COPYCHUNK until now, as that's what Windows Server 2k12 uses for copies initiated by Explorer. I've just sent out the trivial patches adding FSCTL_SRV_COPYCHUNK_WRITE support. Cheers, David -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
