Re: [PATCH 2/2] rbd: compression_hint option

[Dongsheng Yang <dongsheng.yang@xxxxxxxxxxxx>]

在 6/2/2020 4:31 PM, Ilya Dryomov 写道:
> On Tue, Jun 2, 2020 at 4:34 AM Dongsheng Yang
> <dongsheng.yang@xxxxxxxxxxxx> wrote:
> > Hi Ilya,
> >
> > 在 6/2/2020 3:58 AM, Ilya Dryomov 写道:
> > > Allow hinting to bluestore if the data should/should not be compressed.
> > > The default is to not hint (compression_hint=none).
> > >
> > > Signed-off-by: Ilya Dryomov <idryomov@xxxxxxxxx>
> > > ---
> > >    drivers/block/rbd.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
> > >    1 file changed, 42 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> > > index b1cd41e671d1..e02089d550a4 100644
> > > --- a/drivers/block/rbd.c
> > > +++ b/drivers/block/rbd.c
> > > @@ -836,6 +836,7 @@ enum {
> > >        Opt_lock_timeout,
> > >        /* int args above */
> > >        Opt_pool_ns,
> > > +     Opt_compression_hint,
> > >        /* string args above */
> > >        Opt_read_only,
> > >        Opt_read_write,
> > > @@ -844,8 +845,23 @@ enum {
> > >        Opt_notrim,
> > >    };
> > >
> > > +enum {
> > > +     Opt_compression_hint_none,
> > > +     Opt_compression_hint_compressible,
> > > +     Opt_compression_hint_incompressible,
> > > +};
> > > +
> > > +static const struct constant_table rbd_param_compression_hint[] = {
> > > +     {"none",                Opt_compression_hint_none},
> > > +     {"compressible",        Opt_compression_hint_compressible},
> > > +     {"incompressible",      Opt_compression_hint_incompressible},
> > > +     {}
> > > +};
> > > +
> > >    static const struct fs_parameter_spec rbd_parameters[] = {
> > >        fsparam_u32     ("alloc_size",                  Opt_alloc_size),
> > > +     fsparam_enum    ("compression_hint",            Opt_compression_hint,
> > > +                      rbd_param_compression_hint),
> > >        fsparam_flag    ("exclusive",                   Opt_exclusive),
> > >        fsparam_flag    ("lock_on_read",                Opt_lock_on_read),
> > >        fsparam_u32     ("lock_timeout",                Opt_lock_timeout),
> > > @@ -867,6 +883,8 @@ struct rbd_options {
> > >        bool    lock_on_read;
> > >        bool    exclusive;
> > >        bool    trim;
> > > +
> > > +     u32 alloc_hint_flags;  /* CEPH_OSD_OP_ALLOC_HINT_FLAG_* */
> > >    };
> > >
> > >    #define RBD_QUEUE_DEPTH_DEFAULT     BLKDEV_MAX_RQ
> > > @@ -2254,7 +2272,7 @@ static void __rbd_osd_setup_write_ops(struct ceph_osd_request *osd_req,
> > >                osd_req_op_alloc_hint_init(osd_req, which++,
> > >                                           rbd_dev->layout.object_size,
> > >                                           rbd_dev->layout.object_size,
> > > -                                        0);
> > > +                                        rbd_dev->opts->alloc_hint_flags);
> > >        }
> > >
> > >        if (rbd_obj_is_entire(obj_req))
> > > @@ -6332,6 +6350,29 @@ static int rbd_parse_param(struct fs_parameter *param,
> > >                pctx->spec->pool_ns = param->string;
> > >                param->string = NULL;
> > >                break;
> > > +     case Opt_compression_hint:
> > > +             switch (result.uint_32) {
> > > +             case Opt_compression_hint_none:
> > > +                     opt->alloc_hint_flags &=
> > > +                         ~(CEPH_OSD_ALLOC_HINT_FLAG_COMPRESSIBLE |
> > > +                           CEPH_OSD_ALLOC_HINT_FLAG_INCOMPRESSIBLE);
> > > +                     break;
> > > +             case Opt_compression_hint_compressible:
> > > +                     opt->alloc_hint_flags |=
> > > +                         CEPH_OSD_ALLOC_HINT_FLAG_COMPRESSIBLE;
> > > +                     opt->alloc_hint_flags &=
> > > +                         ~CEPH_OSD_ALLOC_HINT_FLAG_INCOMPRESSIBLE;
> > > +                     break;
> > > +             case Opt_compression_hint_incompressible:
> > > +                     opt->alloc_hint_flags |=
> > > +                         CEPH_OSD_ALLOC_HINT_FLAG_INCOMPRESSIBLE;
> > > +                     opt->alloc_hint_flags &=
> > > +                         ~CEPH_OSD_ALLOC_HINT_FLAG_COMPRESSIBLE;
> > > +                     break;
> >
> > Just one little question here,
> >
> > (1) none opt means clear compressible related bits in hint flags, then
> > lets the compressor in bluestore to decide compress or not.
> >
> > (2) compressible opt means set compressible bit and clear incompressible bit
> >
> > (3) incompressible opt means set incompressible bit and clear
> > compressible bit
> >
> >
> > Is there any scenario that alloc_hint_flags is not zero filled before
> > rbd_parse_param(), then we have to clear the unexpected bit?
> Hi Dongsheng,
>
> This is to handle the case when the map option string has multiple
> compression_hint options:
>
>    name=admin,...,compression_hint=compressible,...,compression_hint=none
>
> The last one wins and we always end up with either zero or just one
> of the flags set, not both.


Hi Ilya,

     Considering this case, should we make this kind of useage invalid? 
Maybe we

can do it in another patch to solve all rbd parameters conflicting problem.


Thanx

> Thanks,
>
>                  Ilya