On Tue, 2014-03-11 at 07:08 -0400, Jeff Layton wrote: > On Tue, 11 Mar 2014 10:25:49 +0000 > Sachin Prabhu <sprabhu@xxxxxxxxxx> wrote: > > > On Mon, 2014-03-10 at 14:16 -0400, Jeffrey Layton wrote: > > > On Mon, 10 Mar 2014 16:21:10 +0000 > > > Sachin Prabhu <sprabhu@xxxxxxxxxx> wrote: > > > > > > > On Mon, 2014-03-10 at 11:06 -0400, Jeff Layton wrote: > > > > > On Fri, 7 Mar 2014 13:29:19 +0000 > > > > > Sachin Prabhu <sprabhu@xxxxxxxxxx> wrote: > > > > > > > > > > > We need to add protocol specific calls to the oplock break thread. > > > > > > > > > > > > > > > > I'm not necessarily doubting this, but why? AFAICT, the oplock_break > > > > > calls look more or less identical... > > > > > > > > The oplock values for level 2 oplocks are different. For the 2nd fix > > > > to work, we need to downgrade the oplock only once we are sure that no > > > > writers are operating. > > > > > > > > We need to downgrade oplocks. Since the constants used to describe > > > > oplock states varies between cifs and smb2, we need to abstract out > > > > the functions required to downgrade the oplocks. For this we need > > > > version specific functions. This could either be the oplock_break > > > > function as a whole or we could define a new function called > > > > downgrade_oplock() in smb_version_operations. I have chosen to use > > > > version specific oplock_break here. downgrade_oplock() could instead > > > > be used here which would require fewer changes. > > > > > > > > Should I implement this using a downgrade_oplock() function instead? > > > > > > > > > > I guess I'm unclear on why the set_oplock_level operation (possibly > > > combined with some new struct smb_version_values fields) wouldn't be a > > > better solution. > > > > > > I'll grant however that the oplock handing code is a bit of a mess. It > > > seems like it could benefit from some rethink and redesign... > > > > > > > The constants used to denote oplock values are not consistent across the > > versions > > > > In this case, a level 2 oplock for smb1 is > > #define OPLOCK_READ 3 /* level 2 oplock */ > > for smb2, it is > > #define SMB2_OPLOCK_LEVEL_II 0x01 > > > > similarly the values for other states are different too. > > > > We could abstract these but it sounded like overkill in this situation. > > > > Sachin Prabhu > > > > > > Yeah, that's the part that's a mess. We have a generic set_oplock_level > operation but that requires you to pass down oplock levels that are > version specific. Typically when you have a generic operation, you want > it to take a set of generic values as arguments. I have a patch ready with the downgrade_oplock version specific function. Since this problem leads to data corruption and needs to be included in stable, I propose we go ahead with this version for now. The other option to fix the set_oplock_levels() call will require a lot more changes and may not be suitable for the stable kernels. Once this issue has been fixed, we can work to fix set_oplock_level and remove downgrade_oplock(). Sachin Prabhu -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
