| Message ID | 20210118163006.61659-1-dgilbert@interlog.com |
|---|---|
| Headers | show |
| Series | scatterlist: add new capabilities | expand |
On 2021-01-18 6:27 p.m., David Disseldorp wrote: > 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 <bostroesser@gmail.com> >> Signed-off-by: Douglas Gilbert <dgilbert@interlog.com> >> --- >> 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 <ddiss@suse.de> 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(...) ? Doug Gilbert
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 <ddiss@suse.de> > > 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
Scatter-gather lists (sgl_s) are frequently used as data carriers in the block layer. For example the SCSI and NVMe subsystems interchange data with the block layer using sgl_s. The sgl API is declared in <linux/scatterlist.h> The author has extended these transient sgl use cases to a store (i.e. a ramdisk) in the scsi_debug driver. Other new potential uses of sgl_s could be for the target subsystem. When this extra step is taken, the need to copy between sgl_s becomes apparent. The patchset adds sgl_copy_sgl(), sgl_compare_sgl() and sgl_memset(). The existing sgl_alloc_order() function can be seen as a replacement for vmalloc() for large, long-term allocations. For what seems like no good reason, sgl_alloc_order() currently restricts its total allocation to less than or equal to 4 GiB. vmalloc() has no such restriction. Changes since v5 [posted 20201228]: - incorporate review requests from Jason Gunthorpe - replace integer overflow detection code in sgl_alloc_order() with a pre-condition statement - rebase on lk 5.11.0-rc4 Changes since v4 [posted 20201105]: - rebase on lk 5.10.0-rc2 Changes since v3 [posted 20201019]: - re-instate check on integer overflow of nent calculation in sgl_alloc_order(). Do it in such a way as to not limit the overall sgl size to 4 GiB - introduce sgl_compare_sgl_idx() helper function that, if requested and if a miscompare is detected, will yield the byte index of the first miscompare. - add Reviewed-by tags from Bodo Stroesser - rebase on lk 5.10.0-rc2 [was on lk 5.9.0] Changes since v2 [posted 20201018]: - remove unneeded lines from sgl_memset() definition. - change sg_zero_buffer() to call sgl_memset() as the former is a subset. Changes since v1 [posted 20201016]: - Bodo Stroesser pointed out a problem with the nesting of kmap_atomic() [called via sg_miter_next()] and kunmap_atomic() calls [called via sg_miter_stop()] and proposed a solution that simplifies the previous code. - the new implementation of the three functions has shorter periods when pre-emption is disabled (but has more them). This should make operations on large sgl_s more pre-emption "friendly" with a relatively small performance hit. - sgl_memset return type changed from void to size_t and is the number of bytes actually (over)written. That number is needed anyway internally so may as well return it as it may be useful to the caller. This patchset is against lk 5.11.0-rc4 Douglas Gilbert (4): sgl_alloc_order: remove 4 GiB limit, sgl_free() warning scatterlist: add sgl_copy_sgl() function scatterlist: add sgl_compare_sgl() function scatterlist: add sgl_memset() include/linux/scatterlist.h | 33 ++++- lib/scatterlist.c | 253 +++++++++++++++++++++++++++++++----- 2 files changed, 253 insertions(+), 33 deletions(-)