linux-kernel - Re: [PATCH 1/2] libceph: have ceph_osdc_copy

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <YYKdfvmeqP98Tjno@suse.de>
Date:   Wed, 3 Nov 2021 14:32:30 +0000
From:   Luís Henriques <lhenriques@...e.de>
To:     Jeff Layton <jlayton@...nel.org>
Cc:     Ilya Dryomov <idryomov@...il.com>, Xiubo Li <xiubli@...hat.com>,
        Patrick Donnelly <pdonnell@...hat.com>,
        ceph-devel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] libceph: have ceph_osdc_copy_from() return the osd
 request

On Wed, Nov 03, 2021 at 09:06:31AM -0400, Jeff Layton wrote:
<...>
> > diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c
> > index ff8624a7c964..78384b431748 100644
> > --- a/net/ceph/osd_client.c
> > +++ b/net/ceph/osd_client.c
> > @@ -5347,23 +5347,24 @@ static int osd_req_op_copy_from_init(struct ceph_osd_request *req,
> >  	return 0;
> >  }
> >  
> > -int ceph_osdc_copy_from(struct ceph_osd_client *osdc,
> > -			u64 src_snapid, u64 src_version,
> > -			struct ceph_object_id *src_oid,
> > -			struct ceph_object_locator *src_oloc,
> > -			u32 src_fadvise_flags,
> > -			struct ceph_object_id *dst_oid,
> > -			struct ceph_object_locator *dst_oloc,
> > -			u32 dst_fadvise_flags,
> > -			u32 truncate_seq, u64 truncate_size,
> > -			u8 copy_from_flags)
> > +struct ceph_osd_request *
> > +ceph_osdc_copy_from(struct ceph_osd_client *osdc,
> > +		    u64 src_snapid, u64 src_version,
> > +		    struct ceph_object_id *src_oid,
> > +		    struct ceph_object_locator *src_oloc,
> > +		    u32 src_fadvise_flags,
> > +		    struct ceph_object_id *dst_oid,
> > +		    struct ceph_object_locator *dst_oloc,
> > +		    u32 dst_fadvise_flags,
> > +		    u32 truncate_seq, u64 truncate_size,
> > +		    u8 copy_from_flags)
> >  {
> >  	struct ceph_osd_request *req;
> >  	int ret;
> >  
> >  	req = ceph_osdc_alloc_request(osdc, NULL, 1, false, GFP_KERNEL);
> >  	if (!req)
> > -		return -ENOMEM;
> > +		return ERR_PTR(-ENOMEM);
> >  
> >  	req->r_flags = CEPH_OSD_FLAG_WRITE;
> >  
> > @@ -5382,11 +5383,11 @@ int ceph_osdc_copy_from(struct ceph_osd_client *osdc,
> >  		goto out;
> >  
> >  	ceph_osdc_start_request(osdc, req, false);
> > -	ret = ceph_osdc_wait_request(osdc, req);
> > +	return req;
> >  
> 
> I don't really understand why this function is part of net/ceph. It's
> odd in that it basically allocates, initializes and starts the request
> and then passes back the pointer to it. That doesn't really follow the
> pattern of the other OSD READ/WRITE ops, where we basically set them up
> and start them more directly from the fs/ceph code.
> 
> I think it'd make more sense to just get rid of ceph_osdc_copy_from
> altogether, export the osd_req_op_copy_from_init symbol and open-code
> the whole thing in ceph_do_objects_copy. This isn't otherwise called
> from rbd or anything else so I don't see the benefit of keeping this
> function as part of libceph.

At the time ceph_osdc_copy_from() was implemented, it looked like
something that would make sense to keep on the osd client code, and
eventually useful for rbd too.  But since that didn't happen, this patch
shows that it makes sense to just move the code.  So, yeah, sure.  I'll do
that.  Thanks.

Cheers,
--
Luís