On Wed, Feb 1, 2017 at 6:47 PM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> On Wed, 2017-02-01 at 15:08 +0100, Ilya Dryomov wrote:
>> On Wed, Feb 1, 2017 at 12:49 PM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
>> > ...and start moving bool flags into it.
>> >
>> > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
>> > ---
>> > fs/ceph/dir.c | 2 +-
>> > fs/ceph/mds_client.c | 2 +-
>> > fs/ceph/mds_client.h | 4 +++-
>> > 3 files changed, 5 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
>> > index d4385563b70a..04fa4ae3deca 100644
>> > --- a/fs/ceph/dir.c
>> > +++ b/fs/ceph/dir.c
>> > @@ -371,7 +371,7 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>> > /* hints to request -> mds selection code */
>> > req->r_direct_mode = USE_AUTH_MDS;
>> > req->r_direct_hash = ceph_frag_value(frag);
>> > - req->r_direct_is_hash = true;
>> > + set_bit(CEPH_MDS_R_DIRECT_IS_HASH, &req->r_req_flags);
>>
>> Hi Jeff,
>>
>> Just a couple of nits:
>>
>> These are atomic -- should probably mention in the commit message why
>> is atomicity needed.
>>
>
> It's not strictly needed for most of this stuff. DIRECT_IS_HASH in
> particular could just use __set_bit.
>
> The rest of the flags, I think are already protected from concurrent
> writes by mutexes in the code. What I'm not sure of is whether they are
> protected by the _same_ lock. That's a requirement if we mix all of the
> flags together into the same word and don't want to use the atomic
> *_bit macros.
>
> I can look and see if that's possible. Even if it's not though, using
> the atomic *_bit macros is generally not that expensive (particularly
> in a situation where we're already using sleeping locks anyway).
Auditing for whether __set_bit, etc can be used instead is probably not
worth it. I'm not saying we shouldn't use atomic bitops here -- just
wanted to get this explanation in the commit message.
Thanks,
Ilya
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
